Skip to content

Fix failing CI in Python 3.8 and install numba/jax on specific runs #326

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jun 2, 2023
Merged

Fix failing CI in Python 3.8 and install numba/jax on specific runs #326

merged 4 commits into from
Jun 2, 2023

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented May 30, 2023 •
edited
Loading

Also closes #322 even though the main goal was to fix the failing CI

@ricardoV94
Copy link
Member Author

ricardoV94 commented May 30, 2023 •
edited
Loading

It seems that in the Python 3.8 CI, BLAS is not getting setup correctly? It fails here:

pytensor/.github/workflows/test.yml

Line 126 in 7223194

python -c 'import pytensor; assert(pytensor.config.blas__ldflags != "")'

@ricardoV94 ricardoV94 added the help wanted Extra attention is needed label May 30, 2023
@ricardoV94 ricardoV94 changed the title Add more informative suggestion when iterating over Tensors Investigate failing CI in Python 3.8 May 30, 2023
@maresb
Copy link
Contributor

maresb commented May 30, 2023

I don't see blas or cxx-compiler packages being installed. I wonder why this is happening now?

We probably should be using lockfiles.

@ricardoV94
Copy link
Member Author

I think blas is (supposed to be obtained) from the mkl package?

pytensor/.github/workflows/test.yml

Line 120 in 7223194

mamba install --yes -q "python~=${PYTHON_VERSION}=*_cpython" mkl numpy scipy pip mkl-service graphviz cython pytest coverage pytest-cov pytest-benchmark sympy

It is definitely working for the 3.11 run...

@ricardoV94
Copy link
Member Author

The difference seems to be that it's installing numpy from pypi?

https://github.com/pymc-devs/pytensor/actions/runs/5121096515/jobs/9208464120?pr=326#step:5:92

Installing collected packages: toolz, numpy, filelock, logical-unification, cons, etuples, miniKanren, pytensor
  Attempting uninstall: numpy
    Found existing installation: numpy 1.24.3
    Uninstalling numpy-1.24.3:
      Successfully uninstalled numpy-1.24.3

https://github.com/pymc-devs/pytensor/actions/runs/5121096515/jobs/9208464120?pr=326#step:5:197

numpy                     1.22.4                   pypi_0    pypi

Whereas it did not do this in a previous successful run:
https://github.com/pymc-devs/pytensor/actions/runs/5079024125/jobs/9124296946?pr=280#step:5:190

@ricardoV94
Copy link
Member Author

This is my suspect: https://github.com/conda-forge/numba-scipy-feedstock/pull/4/files

@ricardoV94
Copy link
Member Author

ricardoV94 commented May 31, 2023 •
edited
Loading

This is my suspect: https://github.com/conda-forge/numba-scipy-feedstock/pull/4/files

Yes that's the problem. The new upper pin on scipy 1.7.3 causes that to be chosen, and I guess it now pins numpy from above whereas the old scipy 1.6.2 did not.

@ricardoV94 ricardoV94 force-pushed the test_ci branch 2 times, most recently from f6ea79c to d306c6f Compare May 31, 2023 09:26
@maresb
Copy link
Contributor

maresb commented May 31, 2023

Nice detective work! Here's the upper pin on numpy:
conda-forge/scipy-feedstock#219

But why would this lead to pip downgrading numpy?

@ricardoV94 ricardoV94 force-pushed the test_ci branch 2 times, most recently from 44889f0 to 862222f Compare May 31, 2023 09:44
@ricardoV94 ricardoV94 added GitHub CI/CD no releasenotes and removed help wanted Extra attention is needed labels May 31, 2023
@ricardoV94
Copy link
Member Author

ricardoV94 commented May 31, 2023 •
edited
Loading

Nice detective work! Here's the upper pin on numpy: conda-forge/scipy-feedstock#219

But why would this lead to pip downgrading numpy?

To the best of my understanding when doing pip install -e ., pip sees that PyTensor has a dependency on scipy -> figures out numpy is incompatible -> downgrades numpy. Why pip does this but not mamba I don't know.

I fixed it by forcing the numpy upper-bound when installing numba on the CI. I checked it worked and now pushed a second commit that only installs numba/ jax in their own jobs to address #322

@ricardoV94 ricardoV94 changed the title Investigate failing CI in Python 3.8 Fix failing CI in Python 3.8 and install numba/jax on specific runs May 31, 2023
@ricardoV94
Copy link
Member Author

ricardoV94 commented May 31, 2023 •
edited
Loading

Nice detective work! Here's the upper pin on numpy: conda-forge/scipy-feedstock#219

Specifically, this one to the 1.7 branch: conda-forge/scipy-feedstock#217

I don't think they ever did it to 1.6, so we were not seeing this problem before

@ricardoV94 ricardoV94 force-pushed the test_ci branch 5 times, most recently from 42f11f9 to 50b4ac4 Compare May 31, 2023 10:44
@maresb
Copy link
Contributor

maresb commented May 31, 2023

Specifically, this one to the 1.7 branch

Ah, thanks!!! This is the crucial piece I was missing; I was looking at main.

@ricardoV94 ricardoV94 requested a review from maresb June 1, 2023 11:49
maresb
maresb approved these changes Jun 1, 2023
View reviewed changes
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.

A few stylistic wishes, but LGTM

I found 2c91ff7 a little bit confusing because it almost cherrypicks the reverted 277559b, but not quite. (It would have been nice to see 2c91ff7 cherry-picked in one commit and then tweaked in the next.)

I wonder if it makes sense to inline the commit message of a1dd6f5 because otherwise you have to look in the blame to see the explanation for the numpy upper bound.

@ricardoV94
Copy link
Member Author

ricardoV94 commented Jun 1, 2023 •
edited
Loading

2c91ff7 looks superficially similar but the goal is something totally different: Run both JAX and NUMBA on their own CIs to address #322

I'll inline the comment, hopefully yaml doesn't complain

ricardoV94 added 2 commits June 1, 2023 14:49
@ricardoV94
Pin numpy upper bound in numba install
45c2153 
numba-scipy downgrades the installed scipy to 1.7.3 in Python 3.8, but not numpy, even though scipy 1.7 requires numpy<1.23. When installing PyTensor next, pip installs a lower version of numpy via the PyPI.
@ricardoV94
Run numba and jax tests in separate jobs
a5f2b66
@ricardoV94
Copy link
Member Author

I guess my comment in invalid yaml syntax? https://github.com/pymc-devs/pytensor/actions/runs/5144528955/workflow#L138

@maresb
Copy link
Contributor

maresb commented Jun 1, 2023

The indentation looks really weird. I bet it'll work once you bring it to the right indentation level. Also you should probably split that across multiple lines.

@ricardoV94
Copy link
Member Author

The indentation looks really weird. I bet it'll work once you bring it to the right indentation level. Also you should probably split that across multiple lines.

My editor showed it as being invalid with indentation...

@maresb
Copy link
Contributor

maresb commented Jun 1, 2023

VS Code says invalid as-is, but valid indented.

@ricardoV94
Copy link
Member Author

Wanna push? Away from PC now

@ricardoV94 ricardoV94 merged commit 236a3df into pymc-devs:main Jun 2, 2023
@ricardoV94
Copy link
Member Author

Thanks @maresb

@ricardoV94 ricardoV94 deleted the test_ci branch June 21, 2023 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maresb maresb maresb approved these changes

Assignees
No one assigned
Labels
GitHub CI/CD no releasenotes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Test Numba/ JAX in separate CI
2 participants
@ricardoV94 @maresb