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

Fix github action job: Version 3.9 with arch x64 not found #247

Merged
merged 11 commits into from
May 10, 2024

Conversation

rugeli
Copy link
Collaborator

@rugeli rugeli commented May 8, 2024

Merge this pr first

Problem

What is the problem this work solves, including
CI/CD issue: Error: Version 3.9 with arch x64 not found

Solution

What I/we did to solve this problem
Made a few adjustments, and turns out using python 3.10 - currently the most stable version for cellPACK.
updated checkout and setup-python

Type of change

Please delete options that are not relevant.

  • Bug fix and version update (non-breaking change which fixes an issue)

Copy link

github-actions bot commented May 8, 2024

Packing analysis report

Analysis for packing results located at cellpack/tests/outputs/test_spheres/spheresSST

Ingredient name Encapsulating radius Average number packed
ext_A 25 236.0

Packing image

Packing image

Distance analysis

Expected minimum distance: 50.00
Actual minimum distance: 50.01

Ingredient key Pairwise distance distribution
ext_A Distance distribution ext_A

@rugeli rugeli marked this pull request as draft May 8, 2024 18:37
@rugeli rugeli marked this pull request as ready for review May 8, 2024 19:49
@rugeli rugeli requested review from mogres and meganrm May 8, 2024 19:49
@mogres
Copy link
Collaborator

mogres commented May 8, 2024

Is there any reason we are using Python 3.10 for testing? The base cellpack environment still uses Python 3.9

@@ -53,7 +53,7 @@ jobs:
- name: Set up Python
uses: actions/setup-python@v5
with:
python-version: 3.9
python-version: "3.10"
Copy link
Member

Choose a reason for hiding this comment

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

you could define this as a variable that you then reference in the matrix and down here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I just realized sharing values across jobs might require some tweaks, like adding another job to define and output the python version and using needs in other jobs. The way we have it now looks cleaner.

@rugeli
Copy link
Collaborator Author

rugeli commented May 8, 2024

Is there any reason we are using Python 3.10 for testing? The base cellpack environment still uses Python 3.9

Because one of our github actions failed, with the error described in the title, it seems that the version of ubuntu-latest macos-latest is the cause.

To solve the issue, we can either specify an older ubuntu macos version(20.04) or upgrade the python version, and I chose the latter solution in this pr. I tried a few python versions but ran into issues with deprecations and module not found, version 3.10 finally passed all the tests.

We may need to upgrade the base environment to python 3.10 soon to maintain consistency.

@mogres
Copy link
Collaborator

mogres commented May 9, 2024

To solve the issue, we can either specify an older ubuntu version(20.04) or upgrade the python version, and I chose the latter solution in this pr. I tried a few python versions but ran into issues with deprecations and module not found, version 3.10 finally passed all the tests.

We may need to upgrade the base environment to python 3.10 soon to maintain consistency.

While I do agree we should move to python 3.10 eventually, I am not sure if this is the right approach. The ideal path would be to first create a branch to migrate cellPACK to Python 3.10, check whether this breaks anything beyond our unit tests and then upgrade our CI workflows, requirements, etc. to use the new python version.

Another reasoning for sticking with python 3.9 is because the cellPACK package is a dependency in other analysis packages including simulariumio and cellpack-analysis and we want to make sure these work as expected when we eventually release a newer version with python 3.10.

For the time being, I would recommend using Ubuntu 20.04 in our tests to be compatible with the base cellPACK version. If that is not feasible, we can discuss ways to ensure that the tests are run on the same python version as the base package in one of the WG meetings.

@@ -80,7 +80,7 @@ jobs:
- name: Set up Python
uses: actions/setup-python@v1
with:
python-version: 3.9
python-version: "3.10"
Copy link
Collaborator

@mogres mogres May 9, 2024

Choose a reason for hiding this comment

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

Updating the setup-python version here might also be useful

Added a suggestion comment for this here

@rugeli
Copy link
Collaborator Author

rugeli commented May 9, 2024

To solve the issue, we can either specify an older ubuntu version(20.04) or upgrade the python version, and I chose the latter solution in this pr. I tried a few python versions but ran into issues with deprecations and module not found, version 3.10 finally passed all the tests.
We may need to upgrade the base environment to python 3.10 soon to maintain consistency.

While I do agree we should move to python 3.10 eventually, I am not sure if this is the right approach. The ideal path would be to first create a branch to migrate cellPACK to Python 3.10, check whether this breaks anything beyond our unit tests and then upgrade our CI workflows, requirements, etc. to use the new python version.

Another reasoning for sticking with python 3.9 is because the cellPACK package is a dependency in other analysis packages including simulariumio and cellpack-analysis and we want to make sure these work as expected when we eventually release a newer version with python 3.10.

For the time being, I would recommend using Ubuntu 20.04 in our tests to be compatible with the base cellPACK version. If that is not feasible, we can discuss ways to ensure that the tests are run on the same python version as the base package in one of the WG meetings.

I see your concern, sure I'll try a different approach and we can discuss it at our next meeting.

Copy link
Collaborator

@mogres mogres left a comment

Choose a reason for hiding this comment

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

Maybe updating the setup-python version would help?

@@ -80,7 +80,7 @@ jobs:
- name: Set up Python
uses: actions/setup-python@v1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
uses: actions/setup-python@v1
uses: actions/setup-python@v5

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you are absolutely right!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yay!

@rugeli rugeli requested review from mogres and meganrm May 9, 2024 21:52
@rugeli rugeli merged commit 5c39c55 into main May 10, 2024
7 checks passed
@rugeli rugeli deleted the update/python_version branch May 10, 2024 21:28
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