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

TST: simplify cartopy integration testing (using cartopy 0.22) #4621

Merged
merged 2 commits into from
Aug 8, 2023

Conversation

neutrinoceros
Copy link
Member

Cartopy 0.22 was just released and is supposed to resolve a couple workarounds we've been using to test it.
Most notably, this version doesn't depend on GEOS, and as such, it can afford to have wheels, which should allow us to properly test it on all platforms for cheap !

@neutrinoceros neutrinoceros added tests: running tests Issues with the test setup enhancement Making something better labels Aug 5, 2023
@neutrinoceros neutrinoceros force-pushed the cartopy_on_wheels branch 6 times, most recently from 8ecc8bb to fed64eb Compare August 5, 2023 10:31
@neutrinoceros neutrinoceros marked this pull request as ready for review August 5, 2023 11:19
@neutrinoceros neutrinoceros added this to the 4.3.0 milestone Aug 5, 2023
Copy link
Contributor

@chrishavlin chrishavlin left a comment

Choose a reason for hiding this comment

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

The ci_install.sh script was still installing geos -- which we shouldn't have to do anymore. I left comments pointing out those spots as well as a couple other further simplifications that I think are possible.

Also, adding cartopy>=0.22 as a proper dependency in yt[full] would further simplify the setup here. Thoughts on that?

tests/ci_install.sh Outdated Show resolved Hide resolved
tests/ci_install.sh Outdated Show resolved Hide resolved
tests/ci_install.sh Outdated Show resolved Hide resolved
tests/ci_install.sh Outdated Show resolved Hide resolved
tests/ci_install.sh Outdated Show resolved Hide resolved
tests/ci_install.sh Show resolved Hide resolved
@neutrinoceros
Copy link
Member Author

neutrinoceros commented Aug 7, 2023

The ci_install.sh script was still installing geos -- which we shouldn't have to do anymore. I left comments pointing out those spots as well as a couple other further simplifications that I think are possible.

yes, that is also what I assumed at first but unfortunately geos is still needed to build shapely (which is still a direct dependency to cartopy).

Also, adding cartopy>=0.22 as a proper dependency in yt[full] would further simplify the setup here. Thoughts on that?

this was also one of my goals for this PR but given how shapely is still a pain to install (the source distribution from PyPI doesn't build on Windows), I renounced that goal too;

@chrishavlin
Copy link
Contributor

Oh huh, that's a shame... I think all my comments can be disregarded in that case.

@neutrinoceros
Copy link
Member Author

I checked, and I think the title of this PR says it all

@chrishavlin
Copy link
Contributor

Looking more carefully at shapely -- they do package geos. Did you try removing the geos install from the Ubuntu and Mac branch of the install script?

@chrishavlin
Copy link
Contributor

I checked, and I think the title of this PR says it all

But the shapely docs make it pretty clear that you should only need to install geos explicitly if you want a specific version https://shapely.readthedocs.io/en/latest/installation.html maybe that's not the case in practice

@neutrinoceros
Copy link
Member Author

ah ! Let's try not to build shapely from source then: it doesn't seem to be recommended any more by cartopy, and I assume it would save us installing geos too

@neutrinoceros neutrinoceros marked this pull request as draft August 7, 2023 14:30
@neutrinoceros
Copy link
Member Author

Looks like it worked ! thank you for spotting this Chris, I will iterate a little more and then reopen for review

@neutrinoceros
Copy link
Member Author

Note that wheels are only available for Python 3.9 and above. I don't think this is blocking here, but that's a sign that supporting Python 3.8 is becoming harder for us.

@neutrinoceros
Copy link
Member Author

Another note: I think this works makes conda useless in CI (only used on Windows), but I'd prefer to defer dropping it to a follow-up PR.

@neutrinoceros neutrinoceros marked this pull request as ready for review August 7, 2023 17:07
Copy link
Contributor

@chrishavlin chrishavlin left a comment

Choose a reason for hiding this comment

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

Nice! And removing conda if possible in a later pr makes sense to me.

@neutrinoceros neutrinoceros merged commit 43d32a6 into yt-project:main Aug 8, 2023
12 checks passed
@neutrinoceros neutrinoceros deleted the cartopy_on_wheels branch August 8, 2023 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Making something better tests: running tests Issues with the test setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants