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

Pre commit #17

Merged
merged 16 commits into from
Aug 28, 2020
Merged

Pre commit #17

merged 16 commits into from
Aug 28, 2020

Conversation

mnlevy1981
Copy link
Contributor

Adding pre-commit to the environment and a README.md file detailing how to set it up. I want to add pytest via this pull request as well (and have pre-commit run the tests as well), so this will remain a draft PR until I finish that.

Fixes #13

Required downgrading python to 3.7; it looks like maybe a virtualenv issue,
rather than a pre-commit issue?

I also created a very brief README mostly to include instructions on using
pre-commit.
@mnlevy1981
Copy link
Contributor Author

Note that this will downgrade python from 3.8 to 3.7; it looks like there's an issue with virtualenv and py3.8, and even with workarounds like downgrading virtualenv from v20 to v16 I couldn't get things to work.

We want to allow large files in the repo, otherwise we won't be able to update
notebooks.
Just need to run the pre-commit install line from casper rather than cheyenne
(documented this in the README as well)
First set of tests came from Keith's CESM2 carbon cycle paper work (that seemed
like a good source, since it's also where the utils_units.py file came from)
I'm not sure if we'll be able to test this before accepting the PR; there's a
bit of a chicken and egg problem where I can't put this directly on master
since the tests are only on my branch
Beginning to wish there was a way to test this before committing it...
@mnlevy1981 mnlevy1981 marked this pull request as ready for review August 21, 2020 23:19
@mnlevy1981
Copy link
Contributor Author

It takes ~4 minutes to spin up the conda environment and run pytest in the github action; I wonder if conda would load faster without auto-update-conda: true but after the first 7 attempts I think I'm done mucking with the YAML until Monday.

@klindsay28 -- I was getting deprecation warnings from import numpy.matlib as npm in xr_ds_ex.py so I replaced both instances of npm.repmat(days_1yr, 1, nyrs) with np.tile(days_1yr, nyrs) (and removed the import statement for npm); my brief checks by hand make the two calls look equivalent, and all the tests pass with this change. I think that was the only substantial change I made when I copied the testing files over from your other repo.

@mnlevy1981
Copy link
Contributor Author

Also, I'm leaning against having pre-commit run the pytest; I don't want expected failures to hold up the ability to even commit, and I'm a little concerned that as we add unit tests the runtime for pytest would be an unreasonable wait just to get to be able to enter a commit log message.

@mnlevy1981 mnlevy1981 requested a review from klindsay28 August 24, 2020 16:14
@mnlevy1981
Copy link
Contributor Author

I think this one is ready to go, but would appreciate more eyes on it. There's a README with instructions for setting up pre-commit, and then github actions for running black and pytest. The pytest tests are just a starting point, we should probably write more as we continue to develop the code, but I think it's sufficient for this initial PR

rev: 19.10b0
hooks:
- id: black
args: []
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add the following pre-commit hook for formatting notebooks as well:

  - repo: https://github.com/deathbeds/prenotebook
    rev: f5bdb72a400f1a56fe88109936c83aa12cc349fa
    hooks:
      - id: prenotebook
        args:
          [
            '--keep-output',
            '--keep-metadata',
            '--keep-execution-count',
            '--keep-empty',
          ]

Copy link
Contributor

@klindsay28 klindsay28 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have nothing to add beyond the feedback from @andersy005.
Looks good to me.

1. Add prenotebook hook to pre-commit
2. Use pre-commit to run black (and now prenotebook) in Github Action

I also updated the environment to get black from pip instead of conda so that
users will have the latest version in their environment. This avoids a weird
inconsistency where running black through pre-commit behaves differently than
running black from the command line -- as a specific example, conda was
installing black v19.10b, but Github Action was pulling 20.8b1 from pip and the
later version had stricter formatting requirements for docstrings. I updated
pre-commit to match what was being reported from Github Action (before
modifying the action itself), and wanted the same version in environment.yaml.

Note that the prenotebook hook picked up some changes in several notebooks.
@mnlevy1981
Copy link
Contributor Author

As noted in the last commit message, I've implemented two of Anderson's suggestions:

  1. Added a prenotebook hook
  2. Gihub Action now relies on pre-commit

prenotebook did catch a few issues which are fixed on this branch and will likely cause conflicts when merged into other branches... but I think the right thing to do is keep the existing version on the branch and then let pre-commit fix it.

I also updated the environment file to get precommit from pip instead of conda -- I think this is necessary to have some consistency between pre-commit and our (hires-marbl) conda environment. The README file has a short section on how to make sure pre-commit is using the latest version; as far as I can tell, there needs to be some manual intervention to update the version in pre-commit when the version in the environment changes but I don't think that will turn out to be an issue in practice.

@mnlevy1981 mnlevy1981 merged commit 0aeaded into marbl-ecosys:master Aug 28, 2020
@mnlevy1981 mnlevy1981 deleted the pre-commit branch August 28, 2020 19:38
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.

Set up github actions
3 participants