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

fix: limit numpy version to fix ReadTheDocs build #682

Merged
merged 5 commits into from
May 23, 2023

Conversation

victorgarcia98
Copy link
Contributor

@victorgarcia98 victorgarcia98 commented May 12, 2023

Closes #681

This PR limits the numpy version to <0.1.24 to make it compatible with numba==0.56.4.

Please, let me know your thoughts on this :D

ci: run test on latest allowed dependency versions

Signed-off-by: Victor Garcia Reolid <victor@seita.nl>
@victorgarcia98
Copy link
Contributor Author

Running pytest upgrading the dependencies I found another error:

 ERROR  - ImportError: cannot import name 'validate_email_deliverability' from 'email_validator' (/opt/hostedtoolcache/Python/3.9.16/x64/lib/python3.9/site-packages/email_validator/__init__.py)

Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

Cool!

For the testing of upgraded dependencies,

  1. I'm not sure we are now respecting the limits in app.in, as we just do an eager upgrade. I was thinking about a new make target install-upgraded-deps.
  2. This prolongs our CI minute usage, per each push. Ideally, I'd only use this upgrade setting when a new PR is made, not on each push. Not sure how exactly, though.

requirements/app.in Show resolved Hide resolved
.github/workflows/lint-and-test.yml Outdated Show resolved Hide resolved
.github/workflows/lint-and-test.yml Outdated Show resolved Hide resolved
@nhoening
Copy link
Contributor

You could also split the hotfix from the CI feature, if you want.

@nhoening
Copy link
Contributor

On checking if we still support the latest versions of dependencies (the upgrade case) only once per PR:

I see that we mostly run every action twice as we specify on: ["push", "pull_request"]. The latter contains many events in the PR's life, but they should only be executed when we open the PR:

By default, a workflow only runs when a pull_request event's activity type is opened, synchronize, or reopened.

However, as I'm checking a few PRs, this is not what is happening in our case.

We should stop using pull_request in a bare manner here, and try this instead:

on:
  pull_request:
    types: [opened, reopened]

Then, maybe the best approach to use the upgrade variable (or we'll call it test-upgrades-dependencies) by setting its truth value to github.event.action == 'opened'. I didn't find this exact example, so I'm not certain this is how that check should look like ― I read here.

Another interesting example is how to run a job when a PR is approved, but that works a little differently. Just an inspiration.

Copy link
Contributor

@nhoening nhoening left a comment

Choose a reason for hiding this comment

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

  • numba is not a first-level dependency of ours, so for a future Dev to release this limit, we should comment which library initially is responsible (likely the one that depends on numba, or even a specific version of numba).
  • can you also make the PR description simpler again?

Signed-off-by: Victor Garcia Reolid <victor@seita.nl>
@nhoening nhoening merged commit 91e9e45 into main May 23, 2023
@nhoening nhoening deleted the hotfix/pin-numpy-v1.24 branch May 23, 2023 21:54
@Flix6x
Copy link
Contributor

Flix6x commented Jun 9, 2023

No changelog entry required imo. If anyone disagrees, feel free to place back the label.

@Flix6x
Copy link
Contributor

Flix6x commented Jun 9, 2023

Should this be backported, actually?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Continuous Integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ReadTheDocs build fails
3 participants