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

Remove install dependency + support Python 3.9, 3.10 #647

Merged
merged 13 commits into from
Apr 22, 2024

Conversation

minouHub
Copy link
Collaborator

@minouHub minouHub commented Mar 29, 2024

This PR is to attempt to remove dependencies in the install of RADIS. Each push will be tested for one of the requirements of environment.yaml

Also:

  • add limit python<3.9 in setup.py for pip install

@minouHub minouHub linked an issue Mar 30, 2024 that may be closed by this pull request
@minouHub minouHub added this to the 0.15 milestone Mar 30, 2024
environment.yml Outdated Show resolved Hide resolved
@erwanp
Copy link
Member

erwanp commented Apr 8, 2024

Also upgrade publib dependency in requirements.txt to

publib>=0.4.0

I just published a new version of the package (https://pypi.org/project/publib/ ) that fixes a non compatibility with Matplotlib 3.8.0
due to matplotlib/matplotlib#26831

@erwanp
Copy link
Member

erwanp commented Apr 8, 2024

Regarding the test failures, see my comment here: https://github.com/radis/radis/pull/633/files#r1555134045

@codecov-commenter
Copy link

codecov-commenter commented Apr 10, 2024

Codecov Report

Merging #647 (7d8fcf0) into develop (f9eae74) will increase coverage by 0.01%.
The diff coverage is n/a.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #647      +/-   ##
===========================================
+ Coverage    72.92%   72.94%   +0.01%     
===========================================
  Files          148      148              
  Lines        21066    21066              
===========================================
+ Hits         15363    15366       +3     
+ Misses        5703     5700       -3     

@@ -1,7 +1,7 @@
# Installed by "pip install -e .[dev]" as well as "conda env create --file environment.yml"
lxml # parser used for ExoMol website
hjson # Json with comments (for default_radis.json)
publib>=0.4.0 # Plotting styles for Matplotlib. Version, see #647
publib>=0.3.2 # Plotting styles for Matplotlib. Version, update to 0.4.0 needed for matplotlib==3.8. However, only 0.3.2 is compatible with python==3.8 (see #647)
Copy link
Member

Choose a reason for hiding this comment

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

therefore test on python>=3.11 or something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sorry I didn't get it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

vaex is not compatible with python>3.8 (see original post). matplotlib==3.8 requires python>3.8, thus, we should hold this update for the moment

@minouHub
Copy link
Collaborator Author

@erwanp, waiting for your final review on this. I will try to sort the docs by tonight

@minouHub
Copy link
Collaborator Author

To summarize, when vaex will move to a higher python version we will be able to:

  • remove the limit in python<3.9 in setup.py
  • remove python=3.8 in environment.yml
  • upgrade to publib>=0.4.0 in requirements.txt

@erwanp
Copy link
Member

erwanp commented Apr 21, 2024

Which is actively being worked on : vaexio/vaex#2331 (@minouHub register to this PR to know when it is released)

@erwanp erwanp changed the title Remove install dependency Remove install dependency + support Python 3.11, 3.12 Apr 21, 2024
@minouHub
Copy link
Collaborator Author

@erwanp did you change this PR name because you want to wait for vaex? I would have prefer to merge now and put the support of Python 3.11 on a to-do list

@erwanp erwanp changed the title Remove install dependency + support Python 3.11, 3.12 Remove install dependency + support Python 3.9, 3.10 Apr 21, 2024
@erwanp
Copy link
Member

erwanp commented Apr 21, 2024

@minouHub ok I updated the PR title

@minouHub minouHub merged commit 1ddae00 into develop Apr 22, 2024
2 checks passed
@minouHub minouHub deleted the simplified_versions_instal branch April 22, 2024 12:08
@HajimeKawahara
Copy link
Collaborator

@minouHub @erwanp

We have an installation problem because of

  • add limit python<3.9 in setup.py for pip install

What is the reason for this restriction?

HajimeKawahara/exojax#481

@minouHub
Copy link
Collaborator Author

Hello,
This restriction is introduced because vaex cannot be installed with Python > 3.8. Before this PR, the limit was present in the conda requirements but not in pip. In other words, if you did that, the regular pip install failed, see below:

conda create --name test_radis python==3.9 # or anything above 3.9
conda activate test_radis
pip install radis

The problem was that the error message was hard to understand. With this requirement, it's clearer. In the past, you could somehow trick the install with the developer install pip -e . but this is not stable to my opinion.

The main strategy I applied was to wait for vaex to update their package. The alternative if that is a big problem for you is to revert this modification, but remains the installation problem.

@HajimeKawahara
Copy link
Collaborator

@minouHub Thanks. I see. The problem is that jax requires python >3.8. so, for us, it is a big problem. Can you revert this modification for a while if you do not have a big problem?

@minouHub
Copy link
Collaborator Author

oh wait .... it worked .... I'm trying a quick fix in #651. Will keep you updated

@HajimeKawahara
Copy link
Collaborator

Ah, I see. Thanks for the quick check!

@minouHub
Copy link
Collaborator Author

Hello,
Very sorry for the mess. Two mistakes on my side.

  1. vaex is actually compatible with python 3.9 and 3.10. I don't understand how I missed that.
  2. if vaex limits the python version, we don't need to state it on our side

@HajimeKawahara
Copy link
Collaborator

@minouHub thank you so much!

@erwanp
Copy link
Member

erwanp commented Apr 24, 2024

  1. if vaex limits the python version, we don't need to state it on our side

If and only if vaex is installed through Conda and not Pip on our side; which it is=> so all good. Well done for fixing it

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.

Remove cython dependency
4 participants