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

Run CIs in py37 #1518

Merged
merged 6 commits into from
Oct 11, 2021
Merged

Run CIs in py37 #1518

merged 6 commits into from
Oct 11, 2021

Conversation

laserprec
Copy link
Contributor

@laserprec laserprec commented Sep 2, 2021

Description

Extend the CIs to run the build in 3.7:

  • pr-gate runs in python 3.7 only (upgraded from 3.6)
  • nightly build runs in both 3.6 & 3.7
  • nightly build runs on dedicated self-hosted machines only

Thanks to @anargyri, we have a solution to this with conda install numpy-base as documented in this PR #1515. This would require the use of conda in the pipeline that uses virtualenv with tox. I am wondering what would be the minimum changes to the pipeline to incorporate this fix.

@anargyri, if I understand from this thread correctly, we have this numba import issue in py37 because of a specific numpy version fixed by Tensorflow, so if we somehow update tensorflow would this go away?

Not sure if there are other pip-native solutions to this without involving conda.

Update: Thanks to @anargyri, we have a clean pip-native solution to the above problem after #1540 is merged.

Related Issues

#1511

Checklist:

  • I have followed the contribution guidelines and code style for this project.
  • I have added tests covering my contributions.
  • I have updated the documentation accordingly.
  • This PR is being made to staging branch and not to main branch.

@anargyri
Copy link
Collaborator

anargyri commented Sep 6, 2021

@anargyri, if I understand from this thread correctly, we have this numba import issue in py37 because of a specific numpy version fixed by Tensorflow, so if we somehow update tensorflow would this go away?

Not sure if there are other pip-native solutions to this without involving conda.

(This is just a draft PR to replicate the issue. Hopefully we can find a simple solution :))

I suspect that tensorflow upgrade would allow us to upgrade python for venv & virtualenv too, but I am not completely sure. The reason I am not sure is that, while we need numba for some libraries with algos, numba is not included in TF dependencies. But hopefully the higher numpy version will be more compatible with the algos we import.

Regarding pip-native solutions, I haven't found anything that applies to our case on the internet, but you are welcome to search more, maybe there is something about this out there.

@laserprec laserprec force-pushed the laserprec/py37-support branch 2 times, most recently from 7cb3eff to 9e29652 Compare October 6, 2021 18:24
@laserprec laserprec changed the title Try py37 Run the Build in py37 Oct 7, 2021
@laserprec laserprec changed the title Run the Build in py37 Run CIs in py37 Oct 7, 2021
@laserprec laserprec marked this pull request as ready for review October 7, 2021 14:50
@anargyri anargyri self-assigned this Oct 7, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #1518 (34f8b0a) into staging (9f19411) will increase coverage by 0.08%.
The diff coverage is 94.25%.

Impacted file tree graph

@@             Coverage Diff             @@
##           staging    #1518      +/-   ##
===========================================
+ Coverage    62.12%   62.21%   +0.08%     
===========================================
  Files           84       84              
  Lines         8397     8442      +45     
===========================================
+ Hits          5217     5252      +35     
- Misses        3180     3190      +10     
Impacted Files Coverage Δ
recommenders/evaluation/spark_evaluation.py 86.66% <ø> (ø)
recommenders/evaluation/python_evaluation.py 93.68% <94.11%> (-3.21%) ⬇️
recommenders/utils/spark_utils.py 96.15% <100.00%> (+0.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 448982d...34f8b0a. Read the comment docs.

@laserprec laserprec merged commit 2eccb87 into staging Oct 11, 2021
@laserprec laserprec deleted the laserprec/py37-support branch October 15, 2021 16:03
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.

3 participants