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

Fixing versioneer issue #201

Merged

Conversation

jaketanderson
Copy link
Collaborator

The pAPRika package's versions were built with an old version of versioneer, which was giving me errors when running pip install ., so I included versioneer in the development conda environment. With versioneer 0.29 I ran versioneer init and versioneer install which regenerated the files paprika/__init__.py and paprika/_version.py. These changes solved the issue.

I also specified numpy<2 (which I think is currently redundant because pyMBAR requires that) and added jupyter and jupyter_contrib_nbextensions to the test environment, which allows one to run jupyter nbconvert <notebook.ipynb> --to script immediately after creating the environment; this is useful for development because one can quickly turn the tutorial notebooks into scripts then run them.

@jaketanderson
Copy link
Collaborator Author

I'd like CI to run on this PR. Can someone re-enable the CI workflow for the repository please?

@slochower
Copy link
Member

I would avoid pinning numpy if you don't have to.

I think you have to add a reviewer to allow merging. I'm not sure how to enable CI on this repository now that it lives in the Gilson lab space.

@j-wags
Copy link
Collaborator

j-wags commented Sep 5, 2024

There was a "CI was disabled because of inactivity" message on this page, and I clicked an "enable" button to tell it to start again. Maybe push another commit here and see if it runs?

@j-wags
Copy link
Collaborator

j-wags commented Sep 5, 2024

Oh haha apparently I can push commits to your fork. Looks like CI is going now.

This is an attempt to fix the CI failures, which have error message
```python
      from notebook.nbextensions import (
  ModuleNotFoundError: No module named 'notebook.nbextensions'
```
Althought the packages work on my machine for the purpose stated in the first comment in PR GilsonLabUCSD#201,
the CI workflow is unable to properly install/initialize the jupyter nbextensions, causing the
conda installation to fail. After this change, we expect to see CI failing for a different reason.
@j-wags
Copy link
Collaborator

j-wags commented Sep 5, 2024

Looks like codecov is being obnoxious now. I think a GilsonLabUCSD admin (I think the only one is @slochower) will need to do some configuration for codecov.

It's helpful to have continuous codecov reports to get alerted if a whole branch of testing falls offline, but since you're bringing CI back from the dead the old reports probably aren't accessible anyway. So you could probably just comment out the codecov part from the workflow yaml.

@jaketanderson
Copy link
Collaborator Author

jaketanderson commented Sep 5, 2024

Looks like codecov is being obnoxious now. I think a GilsonLabUCSD admin (I think the only one is @slochower) will need to do some configuration for codecov.

I think the codecov issue in the python 3.10 test is because I've made too many codecov requests in too short a time:

[2024-09-05T18:18:46.609Z] ['error'] There was an error running the uploader: Error uploading to [https://codecov.io:](https://codecov.io/) Error: There was an error fetching the storage URL during POST: 429 - {'detail': ErrorDetail(string='Rate limit reached. Please upload with the Codecov repository upload token to resolve issue. Expected time to availability: 1349s.', code='throttled')}

Maybe if I make another cosmetic commit, we won't see the same rate limit issue. As for the python 3.9 test, unit tests are failing: ERROR paprika/tests/test_utils.py - TypeError: unsupported operand type(s) for |: 'type' and 'NoneType'. I'm optimistic that this is straightforward to fix.

Edit:

The codecov upload still failed with the same error message. Maybe we should change to codecov/codecov-action@v4 here? It would need to be edited by someone with perms.

uses: codecov/codecov-action@v3

@slochower
Copy link
Member

slochower commented Sep 5, 2024

Looks like codecov is being obnoxious now. I think a GilsonLabUCSD admin (I think the only one is @slochower) will need to do some configuration for codecov.

It's helpful to have continuous codecov reports to get alerted if a whole branch of testing falls offline, but since you're bringing CI back from the dead the old reports probably aren't accessible anyway. So you could probably just comment out the codecov part from the workflow yaml.

Mike and @jeff231li are also admins. I logged in to codecov -- it doesn't look like it has run recently but I'm not sure why.

image

Maybe you need to regenerate a new codecov token.

@j-wags
Copy link
Collaborator

j-wags commented Sep 5, 2024

Yeah, I think the token needs updating. It looks like codecov is limiting access for people connecting without a token (ie, using the v3 interface).

The OpenFF Toolkit codecov action uses v4 and defines the token as an environment variable, so that will need to be set as a pAPRika repository secret by an admin. https://github.com/openforcefield/openff-toolkit/blob/main/.github/workflows/CI.yml#L169-L174

It's easy for PRs to spiral in scope and become hard to coordinate on. So I recommend @jaketanderson NOT let codecov be a blocker to this PR, and instead comment out the codecov action from the ci.yaml file here entirely. It can be added back in a separate PR.

Copy link

codecov bot commented Sep 5, 2024

Codecov Report

Attention: Patch coverage is 46.97987% with 79 lines in your changes missing coverage. Please review.

Project coverage is 81.85%. Comparing base (17ae4e3) to head (affc777).
Report is 10 commits behind head on master.

Additional details and impacted files

@jaketanderson
Copy link
Collaborator Author

It's easy for PRs to spiral in scope and become hard to coordinate on. So I recommend @jaketanderson NOT let codecov be a blocker to this PR, and instead comment out the codecov action from the ci.yaml file here entirely. It can be added back in a separate PR.

I updated codecov to v4 from v3. I also falsified "fail_ci_if_error" so if codecov has any more complaints, they will still appear in the workflow logs but won't cause the entire CI to fail. But with v4, codecov seems to have resolved its token issue.

At this point, the PR seems to fixed the versioneer issue and also gotten CI up and running (and passing for python 3.8 and 3.10). I think that's all I'll set out to do in this PR. After this one is merged I'll open up another one to fix failing tests for python 3.9.

Copy link
Collaborator

@j-wags j-wags left a comment

Choose a reason for hiding this comment

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

Awesome. Nice work @jaketanderson! I gave it a quick skim and highlighted a minor thing. I'm not sure if I'm a maintainer (so my approval may not actually let you merge) but I figured I'd give a 👍

devtools/conda-envs/test_env.yaml Outdated Show resolved Hide resolved
@j-wags
Copy link
Collaborator

j-wags commented Sep 6, 2024

@slochower or @jeff231li - Jake can't merge until he has write access, could you update the repo settings to give him those?

@jeff231li
Copy link
Collaborator

@jaketanderson I have sent you an invite to the pAPRika repo and the GilsonLab organization.

@jaketanderson jaketanderson merged commit 688cc2c into GilsonLabUCSD:master Sep 7, 2024
5 of 7 checks passed
@jaketanderson jaketanderson deleted the maintenance_testing branch September 7, 2024 01:33
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.

4 participants