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

All the work from the IOOS code sprint #27

Merged
merged 26 commits into from
Jun 6, 2024
Merged

All the work from the IOOS code sprint #27

merged 26 commits into from
Jun 6, 2024

Conversation

ChrisBarker-NOAA
Copy link
Collaborator

Sorry for a bunch of stuff all in one PR -- if you don't want all (or most) or it, I can split it up, but this is what's here:

  • Set up a pixi config -- it can optionally be used for development, etc, but it is used for the CI testing -- which really makes things easy. -- I did add some notes on how to use it to the README.

  • Set up gitHub workflows to test -- a matrix of Windows, Linux, and Mac (not ARM Mac, but I think that can be added fairly easily) and Python 3.9 -- 3.12 (3.9 is failing, but I think there's a change in main that will fix that -- and maybe we can drop it anyway)

  • Added some point in polygon performance testing -- kept it there for posterity

  • Refactored the existing (few) tests to use pytest parametrize

  • Added a couple more tests for the UGRID stuff.

I think that's it :-)

@mpiannucci
Copy link
Collaborator

All in favor getting rid of the 3.9 tests and cutting that version from support? That seems to be the only failure currently

@ChrisBarker-NOAA
Copy link
Collaborator Author

I think they would pass for 3.9 if we brought in the latest changes @omkar-334 made.

But yes, I think we should dump 3.9 I'll do that in the CI soon.

Copy link
Collaborator

@mpiannucci mpiannucci left a comment

Choose a reason for hiding this comment

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

Overall looks really good, i just need to figure out if there is a way to not list out the dependnecies individually three different times. conda and pixi should be smart enough to get that info from the pyprojct

@@ -0,0 +1,27 @@
# requirements file for conda
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this if the dependencies are declared in `pyproject.toml correctly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If everyone is using pixi, then this is totally unneeded.

I put this in before I did the pixi stuff.

So probably delete it, and if we need a way to specify conda requirements outside of pixi, we can auto-generate from the pyproject.toml.


[tool.pixi.tasks]

[tool.pixi.dependencies]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do pixi dependencies need to be listed in addition to the package dependencies? Would love not to have to duplicate

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They do, because there is not a 1:1 relationship between PyPi and conda-forge packages. There are ways to figure out the mapping (grayskull does it) but I don't know how robust it is, and I don't think pixi supports it anyway.

I'm still wrapping my head around what pixi is for, exactly. But I think it's more a "Project" environment manager. So the pixi requirements are really about what you need to do a project with this package -- e.g. develop it, test it, run the examples, etc, rather than what you need to install the package and use it somewhere else.

The requirements in the main part of the pyproject.toml are for putting in with a built package to distribute it. (and what you need to build it).

The conda version of that is the requirements in a conda recipe -- which we may get to once this stabilizes ... (then there will be three copies of the requirements .. sigh)

One thing I think we could simplify is the three pixi features -- do we really need base, and dev and examples? We could simply put the whole thing in base -- or maybe all the dev dependencies in base, and then add an examples one (or not have that at all ...) ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay then im good with how it is for now then! Thank you for the long explanation

@mpiannucci mpiannucci merged commit 2051c05 into main Jun 6, 2024
9 checks passed
@mpiannucci mpiannucci deleted the ioos_sprint branch June 6, 2024 14:54
@ChrisBarker-NOAA
Copy link
Collaborator Author

Thanks!

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.

2 participants