Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Additional tooling #18

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Additional tooling #18

wants to merge 6 commits into from

Conversation

qxcv
Copy link
Member

@qxcv qxcv commented Aug 21, 2020

This cleans up some of the existing tooling and adds some more. Specifically:

  • Removes requirements.txt and adds all deps to setup.py instead.
  • Adds run_rep_learner, il_train and il_test commands to entry_points so that we don't have to do python src/il_representations/scripts/{name}.py.
  • Automatically runs flake8 and isort as part of the unit tests. This uses the pytest-isort and pytest-flake8 plugins.
  • Adds a reformat.sh script that automatically cleans up imports with isort and autoflake, then reformats to be PEP8-compliant with yapf. This can handle most of the problems identified by pytest-isort and pytest-flake8. My hope is that from now on, we can just run reformat.sh before committing but otherwise not have to worry about formatting at all (except wrapping strings and comments).

This PR also reformats ALL the code to be compliant with the new flake8 style. I've rebased everything into two commits: one that makes the tooling changes, and one that makes the (mostly automated) formatting changes. This should make it easier to review.

One potentially controversial choice I've made here is the line length of 100. I noticed that Cody and Cynthia's code was mostly around ~120 columns. I personally prefer shorter line lengths because I can fit more code on my screen; e.g. <=83 columns means I can fit five files, <=105 columns means I can fit four, and <~130 means I can fit three. I chose 100 as a compromise that makes it easy to fit several files on the screen at the same time without forcing us to wrap too much. I'm open to discussions about that choice, as well as discussions about how we can tweak yapf to make files look nice 😄

Another thing: this PR requires everyone to uninstall & reinstall MAGICAL (if they have it installed already). I made some changes that are necessary to work with the latest version of Pyglet, but apparently those won't automatically get installed by pip if you have an existing version 😞

Move all deps to setup.py

Fewer torch conflicts

Fix version conflict

Hopefully fix CircleCI

Forgotten pytest plugins
Base automatically changed from gail-and-augs to master September 7, 2020 22:19
@qxcv qxcv requested a review from decodyng October 7, 2020 01:13
@qxcv
Copy link
Member Author

qxcv commented Oct 7, 2020

(Aside: f0907c5 is a huge batch of (mostly automatic) formatting changes. This PR should therefore be merged after pretrain_n_adapt, so that we can redo that big autoformatting commit.)

@decodyng
Copy link
Member

Finally jumping back to this: it does just unfortunately seem like line length is a competing needs issue, since I find it quite a lot more difficult to parse code in cases where single statements are forced to be spread out over many line breaks for the sake of being under a short line length limit. I expect having a hard limit of 100 would lead to a decent (though not massive) unpleasantness-increase when it comes to my interacting with the codebase.

In general, I think I find the notion of strictly-enforced line length limits difficult to work well with, because there's often a guideline that is fine for 95% of cases, but there is some small set of cases where the change feels arbitrary and actually quite detrimental to the readability of the line (as an example: line 147-148 of the new decoders.py, where I find it a lot more difficult to parse that equation when it's split onto two lines). I'd be fine with having 100 as a norm of "set PyCharm to this and try to stay under it unless it's a strong detriment to readability," but I'm less psyched about having it as a strictly enforced limit by an autoformatter.

Ultimately, though, this is a competing needs thing, and if the other contributors on the project have the similar preference of wanting short line lengths for putting multiple files on a screen (which isn't part of my workflow, except with an external monitor), I'm fine with being outvoted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants