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(ci): Run tests for more python versions #296

Merged
merged 9 commits into from
Mar 8, 2024

Conversation

hassec
Copy link
Contributor

@hassec hassec commented Mar 3, 2024

I wanted to disentangle the fixing of builds for specific python versions from #293.

So this PR keeps the setup.py way of building OMAS and does not yet fix anything in regard to the Cython warnings.

All that this PR does is update the CI to also run the tests for python versions 3.6-3.11,
and apply some small fixes or workarounds to make the OMAS tests pass for these python versions.

We only use OMAS with python >=3.10 so would love to see these tests as part of the CI to ensure everything works as expected on these versions too.

@orso82
Copy link
Member

orso82 commented Mar 4, 2024

Thank you @hassec !! Fantastic! @smithsp @kalling can you please take a look as well? Then I think we can merge!

Copy link

@kalling kalling left a comment

Choose a reason for hiding this comment

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

Looks good as far as I can tell.

# on 3.7 this test raises:
# ValueError: Number of rows must be a positive integer, not 7.0
# It seems like this is caused by a bug in omfit_classes where a float
# instead of int is passed to plot
Copy link

Choose a reason for hiding this comment

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

@orso82 Any idea if this has been fixed on the OMFIT side? I assume we should just cast it?

Copy link
Member

Choose a reason for hiding this comment

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

Probably this needs to be fixed on the OMFIT side. Could you please take care of it @kalling ?

Copy link

Choose a reason for hiding this comment

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

@orso82 Alright.
@hassec Would you mind sending me (or pointing me to) a trace of the error that occurs if you don't skip this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kalling no problem. It's in omfit_gapy.py which has the following lines:

        cplot = max([np.floor(np.sqrt(nplot)), 1.0])
        rplot = np.ceil(nplot / cplot)

which should probably we wrapped in int(...)

Corresponding ci run with the error is here

Copy link

@kalling kalling Mar 4, 2024

Choose a reason for hiding this comment

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

cplot is used in other calculations later that look like they may need it to be a float, so I submitted a change to OMFIT auto_merge that stores the cast versions passed in to subplot as plot_cols and plot_rows instead of doing the cast at assignment, just to be safe. Re-ran the tests locally and it took care of the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kalling are you sure? The latest version on pypi is from Nov 2023

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kalling @orso82 How would you feel about merging as is, and I can follow up as soon as the fix on omfit_classes is released on PyPi? Or if it's coming in the next days I'm also happy to wait.

There isn't really a strong urgency, but I'd like to avoid that the PR sits too long, goes stale and gets forgotten.

Copy link

Choose a reason for hiding this comment

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

@hassec Interesting, I wonder if the 2 factor auth introduced messed up the automation for uploading to pypi. It should currently be at 3.2024.09
I'll take a look.

Copy link

Choose a reason for hiding this comment

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

Ok, I fixed the pypi uploading issue for omfit-classes and 3.2024.9.2 was uploaded. Hopefully now it'll actually be weekly again :) (Assuming tests pass)
Thanks for pointing that out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @kalling I'll have an update PR for OMAS ready asap to remove the workaround :)

Copy link
Member

@smithsp smithsp left a comment

Choose a reason for hiding this comment

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

Please double check that all of the desired tests run for all of the python versions requested.

@hassec
Copy link
Contributor Author

hassec commented Mar 5, 2024

@smithsp was that comment for me? What would you like to see checked aside from what's shown in the CI?

@kalling
Copy link

kalling commented Mar 5, 2024

I verified the tests work locally with those OMFIT changes. Testing here would probably need a new omfit_classes release as @hassec mentioned as a comment in my review.

@smithsp
Copy link
Member

smithsp commented Mar 6, 2024

@hassec regarding #296 (comment) , yes, I am just asking that you double check the CI outputs to be sure the 3.7 tests run the same tests as the other versions.

@hassec
Copy link
Contributor Author

hassec commented Mar 6, 2024

@smithsp yes, on all versions we collect 141 tests.
3.6 skips 13
3.7 skips 14 ( this is the omfit_classes bug we will wait for and as soon as that is on pypi we can remove the skipping of that additional test)
For python >= 3.8 it skips 15 tests. This is higher due to omfit + xarray problem.

@smithsp smithsp merged commit 110ba8b into gafusion:master Mar 8, 2024
6 checks passed
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.

4 participants