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

feat: flex python version to make all 3.10 versions compatible #406

Merged
merged 6 commits into from
Jan 15, 2024

Conversation

d0choa
Copy link
Collaborator

@d0choa d0choa commented Jan 12, 2024

Requiring a very specific version of python package 3.10.8 might be very stringent. This PR attempts to allow the package installation in any 3.10.X version as a first step.

@codecov-commenter
Copy link

codecov-commenter commented Jan 12, 2024

Codecov Report

Attention: 129 lines in your changes are missing coverage. Please review.

Comparison is base (42b366c) 85.67% compared to head (95c3637) 86.20%.
Report is 57 commits behind head on dev.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #406      +/-   ##
==========================================
+ Coverage   85.67%   86.20%   +0.53%     
==========================================
  Files          89       95       +6     
  Lines        2101     2596     +495     
==========================================
+ Hits         1800     2238     +438     
- Misses        301      358      +57     
Files Coverage Δ
src/airflow/dags/common_airflow.py 90.38% <100.00%> (ø)
src/airflow/dags/dag_preprocess.py 100.00% <ø> (ø)
src/airflow/dags/finngen_preprocess.py 100.00% <100.00%> (ø)
src/airflow/dags/gwas_catalog_harmonisation.py 43.47% <ø> (ø)
src/airflow/dags/gwas_curation_update.py 100.00% <100.00%> (ø)
src/otg/cli.py 91.66% <100.00%> (+21.66%) ⬆️
src/otg/common/session.py 87.50% <100.00%> (+0.32%) ⬆️
src/otg/config.py 100.00% <100.00%> (ø)
src/otg/dataset/dataset.py 91.80% <100.00%> (ø)
src/otg/dataset/l2g_feature_matrix.py 82.92% <100.00%> (+7.31%) ⬆️
... and 35 more

@ireneisdoomed ireneisdoomed self-requested a review January 12, 2024 17:55
@d0choa d0choa marked this pull request as draft January 12, 2024 19:47
@d0choa
Copy link
Collaborator Author

d0choa commented Jan 12, 2024

I changed my local python version to 3.10.13 (pyenv + poetry), built a wheel, uploaded it to the bucket, and installed in dataproc (3.10.8). Everything seems to be working without problems.

@d0choa d0choa marked this pull request as ready for review January 12, 2024 21:07
@ireneisdoomed
Copy link
Contributor

You need to change how we parse the Python version in install_dependencies.sh (what we run when we do make setup-dev).
It currently parses the TOML, I think we can simply read from .python-version. Sth like:

echo "Activating Pyenv environment with a Python version required for the project..."
if [ -f ".python-version" ]; then
    PYTHON_VERSION=$(cat .python-version)
else
    echo ".python-version file not found."
    exit 1
fi

For consistency and simplicity, I think it is a good idea to lock the version we use for development (defined in .python-version) and still support different Python subversions for our users (defined in the TOML).
Let me test this a bit more.

Copy link
Contributor

@ireneisdoomed ireneisdoomed left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

@d0choa d0choa merged commit f166a7e into dev Jan 15, 2024
3 checks passed
@d0choa d0choa deleted the do_python_version branch January 15, 2024 12:01
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