Skip to content

Revert "Run numba tests in python 3.10 instead of 3.11" #317

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

Merged
merged 3 commits into from
May 24, 2023

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented May 23, 2023

This reverts commit 277559b and tests numba compatibility with py311

Closes #239

@ricardoV94
Copy link
Member Author

One test with a Categorical seems to fail. I think this was already fixed or marked as ignored upstream in Aesara.

Some tests are xpassing where we can remove the mark.

@ricardoV94
Copy link
Member Author

Should we add the new numba version as a lower dependency?

This PR is not yet using any new functionality from the latest version. Simply marked some tests as xfail and fixed a bug with Categorical that should become irrelevant after #316

@codecov-commenter
Copy link

Codecov Report

Merging #317 (2c3504c) into main (203d919) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #317   +/-   ##
=======================================
  Coverage   80.50%   80.51%           
=======================================
  Files         154      154           
  Lines       45207    45210    +3     
  Branches    11048    11049    +1     
=======================================
+ Hits        36396    36400    +4     
+ Misses       6610     6609    -1     
  Partials     2201     2201           
Impacted Files Coverage Δ
pytensor/link/numba/dispatch/random.py 98.83% <100.00%> (+0.02%) ⬆️

... and 1 file with indirect coverage changes

@michaelosthege
Copy link
Member

Should we add the new numba version as a lower dependency?

For the package or for the test environment?

I would not add it for the package, but for the test environment it might be a good idea

@ricardoV94
Copy link
Member Author

Updated the numba version used in the CI and dev environment

@ricardoV94
Copy link
Member Author

Tests are passing can I get a thumbs up @michaelosthege / @maresb ?

@maresb
Copy link
Contributor

maresb commented May 24, 2023

It looks to me like there are currently no tests running without Numba. If this is the case and I'm not just missing something, maybe it'd make sense to add Numba as a dependency in pyproject.toml?

Copy link
Contributor

@maresb maresb left a comment

Choose a reason for hiding this comment

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

Up to that question about adding Numba as a dep, LGTM.

@ricardoV94
Copy link
Member Author

It looks to me like there are currently no tests running without Numba.

What do you mean? Most tests don't run with numba at all.
Numba is a developer dependency and installed manually in the CI.
Numba is an optional dependency in pyproject.toml as well, just not pinning the latest version, as no feature directly depends on it:

pytensor/pyproject.toml

Lines 102 to 105 in f0fda41

numba = [
"numba>=0.55",
"numba-scipy>=0.3.0"
]

@ricardoV94 ricardoV94 merged commit bb7d70f into pymc-devs:main May 24, 2023
@ricardoV94
Copy link
Member Author

Merging this to unblock other PRs, but let me know if you think we should bump numba min version in the toml as well

@maresb
Copy link
Contributor

maresb commented May 24, 2023

Ah, what I said was a bit ambiguous. I meant about Numba being always installed in all the environments in which pytest is run for the CI testing workflow. Specifically, can you point to any particular pytest run in the test workflow which executes in an environment where numba is not installed?

@ricardoV94
Copy link
Member Author

Ah, what I said was a bit ambiguous. I meant about Numba being always installed in all the environments in which pytest is run for the CI testing workflow. Specifically, can you point to any particular pytest run in the test workflow which executes in an environment where numba is not installed?

Ah, no that doesn't currently happen the way we split the CIs, but we could easily have the numba ones run on their own job. It used to be distinict before

@maresb
Copy link
Contributor

maresb commented May 24, 2023

Ok, we might want to split the CIs if we want tests to cover the case of accidentally triggering Numba when it's not supposed to be used. Would this be useful?

@ricardoV94
Copy link
Member Author

You mean testing that PyTensor works without numba being installed? Yes that could be useful

@maresb
Copy link
Contributor

maresb commented May 24, 2023

Exactly.

@ricardoV94 ricardoV94 deleted the test_numba_py311 branch May 25, 2023 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test Numba with Python 3.11 once it is supported
4 participants