Skip to content

MNT, CI: Update pyproject.toml to include [dependency-groups] - #127

Merged
tylerjereddy merged 5 commits into
mainfrom
aganguly_Issue_123
Sep 15, 2026
Merged

tylerjereddy merged 5 commits into
mainfrom
aganguly_Issue_123

Conversation

@aganguly-lab

@aganguly-lab aganguly-lab commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Fixes #123.

  • The [project.optional-dependencies] list is removed from pyproject.toml
  • The [dependency-groups] list is added to pyproject.toml
  • Two optional groups are added: testing and linting.
  • Changes are made to all .yml files (except wheel.yml and dependabot.yml) to account for the updated workflow.

With this update, there are three commands that can be used to build the project from source:

python -m pip install .
python -m pip install --group testing
python -m pip install --group linting

The first command installs the project as normal. The second installs the dependencies needed to run the test suite. The last installs all dependencies needed to lint (ruff check and numpydoc lint).

When running, make sure pip is up to date. Installation of the testing or linting group will fail if the version of pip is 25.0 or lower. In particular, I found that the default version of pip on darwin and chicoma was too low for this to work, but everything does work when I update pip using pip install --upgrade pip first. I am not sure if this will cause the CI pipeline to fail. If it does, I will add another commit adding the above incantation to the relevant .yml files.

I did not modify wheel.yml. To my understanding, wheel.yml is intended to test the build as seen by users of the code. Users should not be able to directly install the testing/linting versions of the package.

AI Statement: I did not use AI to write any of the code on this branch. I did consult AI extensively with questions about pip installations and builds. I also used AI for debugging purposes.

Testing the update

I have little experience working with installation, so I figured I would include an outline of how I tested my changes to help reviewers. I ran the code below on my laptop and on darwin (I could not secure a chicoma node). For the unit tests, I first ran the code as described below, but then once I confirmed all tests were collected and a few tests had run, I stopped the testing. I then used python -m pytest -n 8 to verify the tests separately (remember to pip install pytest-xdist first).

%%% start from the root of the directory and deactivate any venvs
%%% make sure unit testing works
git clean -xdf
python -m venv test ; source test/bin/activate
python -m pip install --upgrade pip
python -m pip install -v --group testing .
cd
python -m pytest --pyargs gfdl --cov=gfdl --cov-report=term-missing
deactivate
git clean -xdf

Return to the root directory.

%%% test linting
python -m venv test ; source test/bin/activate
python -m pip install --upgrade pip
python -m pip install -v --group linting .
ruff check
numpydoc lint src/gfdl/model.py src/gfdl/activations.py src/gfdl/weights.py
cd docs && make html SPHINXOPTS="-W --keep-going"
deactivate
cd ..
git clean -xdf

%%% make sure building the project is unaffected (no need to upgrade pip)
python -m venv test ; source test/bin/activate
python -m pip install build pytest pytest-cov
python -m build
python -m pip install dist/gfdl*.whl
python -m pip install pandas ucimlrepo
cd 
python -m pytest --pyargs gfdl --cov=gfdl --cov-report=term-missing

Ankan Ganguly added 2 commits September 1, 2026 13:42
… simplify the process of

restoring this version of the code.

* Introduces two new dependency-groups in the pyproject.toml file: testing and linting
* Removed [project.optional-dependencies] from pyproject.toml: none are needed
* Removed the .github/constraints/deps.txt file
* Updated .yml files with new install instructions
I forgot to install the project and its dependencies.

@tylerjereddy tylerjereddy left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks, I added some quick review comments, but I'll let Seth do the main review here.

Comment thread .github/workflows/ci.yml Outdated
Comment thread pyproject.toml
Ankan Ganguly added 2 commits September 2, 2026 18:35
…nse to

#127 (comment) and updated the .yml files
accordingly. Changes are untested, but I included this commit for easy retreval during testing.
…ant. Not sure why SciPy has it. I also removed

some packages in the linting group that belonged purely in the doc group.

@sdtemple sdtemple left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I verified that your commands run Mac OS and on selene (different but same OS from your darwin run).

It would be nice to confirm that the CI failures are from #117. The CI is set by default to fail-fast, whereas with in the .yml file I think we can keep it running for a bit:

    strategy:
      fail-fast: false  # <--- This is the key line

This might not be the desired long term '.yml' file strategy, though. I forked the branch and added this fail-fast: false to the ci.yml file. I also changed on pushes to be "*". I confirmed via this that the base CI failure is from the partial fit test.

Note: this happens for both Windows OS and Ubuntu OS. We might only have been noticing it for Linux because that's what the cluster uses and it is the first crash (temporally) in the CI run.

Comment thread .github/workflows/docs.yml
Comment thread pyproject.toml
Comment thread .github/workflows/docs-deploy.yml
Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/docs.yml
Comment thread pyproject.toml Outdated
Comment thread pyproject.toml Outdated
Comment thread .github/workflows/docs.yml
Comment thread pyproject.toml
Comment thread pyproject.toml Outdated
@tylerjereddy

Copy link
Copy Markdown
Collaborator

Just a suggestion--usually best to only review things that changed in the diff vs. stuff that was already there before re: #127 (comment) for example.

…espectively. Added a version number for pytest (9.1.1) to match the version used by numpy.
@sdtemple

Copy link
Copy Markdown
Collaborator

In your first comment, you mention using python -m pip install --upgrade pip in all the examples of how you tested the code changes. Do you have a reason for why we may need to upgrade to version 26.2.1?

If this is necessary, you might want to add that in requires in the pyproject.toml, but, from my double checking, I didn't run into any issues without using that upgrade pip line. I.e., default pip version 26.1.2 is good.

@aganguly-lab

Copy link
Copy Markdown
Collaborator Author

I couldn't respond directly, so this is a response to #127 (comment).

When I first posted the MR, I was getting low pip versions (I think something like 24.0) on darwin and chicoma. Since pip only added the --group option in version 25.1, the optional builds were not working. For some reason, pip is now up to date there. I think I may have accidentally upgraded pip globally?

@aganguly-lab

aganguly-lab commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator Author

@tylerjereddy
I think @sdtemple has finished his review, and I think I have responded to his review comments to his satisfaction.

@sdtemple

sdtemple commented Sep 14, 2026

Copy link
Copy Markdown
Collaborator

I couldn't respond directly, so this is a response to #127 (comment).

When I first posted the MR, I was getting low pip versions (I think something like 24.0) on darwin and chicoma. Since pip only added the --group option in version 25.1, the optional builds were not working. For some reason, pip is now up to date there. I think I may have accidentally upgraded pip globally?

I see, so your example code for testing the changes is more about the global pip required to install the package, and not related to needing such a version specified in our pyproject.toml. That is, there should be a global pip or environment pip ahead of time to even run our pip install .. Makes sense to me. Thanks for looking into it.

@tylerjereddy

Copy link
Copy Markdown
Collaborator

This PR is being opened from a branch that was pushed to the main repo instead of from your fork. Please don't do that, as noted in gh-108. I'll proceed with doing a round of review here, but if you open another PR from a branch that isn't on your fork, I'll just close it next time.

@tylerjereddy tylerjereddy added this to the 0.3.0 milestone Sep 15, 2026
@tylerjereddy

Copy link
Copy Markdown
Collaborator

I think I may have accidentally upgraded pip globally?

Probably not, we're all regular users on darwin/HPC, without elevated privileges.

I read over the diff here and it seems "ok" now, thanks both. I've added this PR to the 0.3.0 release milestone and I'll merge it shortly. I would say that the original PR description is a bit too verbose perhaps, and some of the commit messages are a bit messy, but at least readable.

It is a bit annoying that CI isn't passing re: gh-117. We need to get that fixed up soon one way or another. That has been plaguing the project for ~3 months now.

@tylerjereddy
tylerjereddy dismissed sdtemple’s stale review September 15, 2026 17:39

Comments were addressed.

@tylerjereddy
tylerjereddy merged commit ff1a114 into main Sep 15, 2026
3 of 19 checks passed
@tylerjereddy
tylerjereddy deleted the aganguly_Issue_123 branch September 15, 2026 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MAINT: modernize to use dependency groups (PEP 735)

3 participants