Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates the forcingprocessor project to use Python 3.10 and migrates the CI workflows from pip to uv for package management. The change modernizes the build toolchain by adopting a faster, more reliable package manager.
- Updated minimum Python version requirement from 3.9 to 3.10
- Migrated all GitHub Actions workflows from pip to uv for dependency installation
- Added virtual environment creation and activation steps in CI workflows
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| forcingprocessor/setup.cfg | Updated minimum Python version requirement to 3.10 |
| .github/workflows/forcingprocessor_weights.yaml | Updated to use Python 3.10 and uv package manager |
| .github/workflows/forcingprocessor_plotting.yaml | Updated to use Python 3.10 and uv package manager |
| .github/workflows/forcingprocessor_output_opts.yaml | Updated to use Python 3.10 and uv package manager |
| .github/workflows/forcingprocessor_gcs_sources.yaml | Updated to use Python 3.10 and uv package manager |
| .github/workflows/forcingprocessor_aws_sources.yaml | Updated to use Python 3.10 and uv package manager |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| source .venv/bin/activate | ||
| uv pip install -e ./forcingprocessor | ||
| uv pip install pytest | ||
|
|
||
| - name: Test with hf2ds | ||
| run: | | ||
| source .venv/bin/activate | ||
| cd forcingprocessor | ||
| python -m pytest -vv tests/test_hf2ds.py |
There was a problem hiding this comment.
The virtual environment activation command source .venv/bin/activate may not persist between shell commands in GitHub Actions. Consider using uv run instead of manual activation, or ensure each command that needs the virtual environment includes the activation in the same step.
| source .venv/bin/activate | |
| uv pip install -e ./forcingprocessor | |
| uv pip install pytest | |
| - name: Test with hf2ds | |
| run: | | |
| source .venv/bin/activate | |
| cd forcingprocessor | |
| python -m pytest -vv tests/test_hf2ds.py | |
| uv pip install -e ./forcingprocessor | |
| uv pip install pytest | |
| - name: Test with hf2ds | |
| run: | | |
| cd forcingprocessor | |
| uv run python -m pytest -vv tests/test_hf2ds.py |
| pip install uv | ||
| uv venv | ||
| source .venv/bin/activate | ||
| uv pip install -e ./forcingprocessor | ||
| uv pip install pytest |
There was a problem hiding this comment.
The virtual environment activation command source .venv/bin/activate may not persist between shell commands in GitHub Actions. Consider using uv run instead of manual activation, or ensure each command that needs the virtual environment includes the activation in the same step.
| pip install uv | ||
| uv venv | ||
| source .venv/bin/activate | ||
| uv pip install -e ./forcingprocessor | ||
| uv pip install pytest |
There was a problem hiding this comment.
The virtual environment activation command source .venv/bin/activate may not persist between shell commands in GitHub Actions. Consider using uv run instead of manual activation, or ensure each command that needs the virtual environment includes the activation in the same step.
| pip install uv | ||
| uv venv | ||
| source .venv/bin/activate | ||
| uv pip install -e ./forcingprocessor | ||
| uv pip install pytest |
There was a problem hiding this comment.
The virtual environment activation command source .venv/bin/activate may not persist between shell commands in GitHub Actions. Consider using uv run instead of manual activation, or ensure each command that needs the virtual environment includes the activation in the same step.
| pip install uv | ||
| uv venv | ||
| source .venv/bin/activate | ||
| uv pip install -e ./forcingprocessor | ||
| uv pip install pytest |
There was a problem hiding this comment.
The virtual environment activation command source .venv/bin/activate may not persist between shell commands in GitHub Actions. Consider using uv run instead of manual activation, or ensure each command that needs the virtual environment includes the activation in the same step.
JordanLaserGit
left a comment
There was a problem hiding this comment.
Thanks @Vishak-V for working on this!
I have just merged a pytest and workflow to main that will run the nrds forcing processing to test that bit directly. Ideally we merge that test into this branch before merging, so we can confirm the upgrade of python from 3.9 to 3.10 doesn't break things. I had observed forcing processing fail for python3.12 on the multiprocessed write to s3.
There looks to be a conflict with datastream-deps and the main branch, so that'll need to be resolved as well.
One suggestion (that we can deal with in a later issue if we prefer) is modifying the forcingprocessing and datastream python tests to use a github runner that has a python version that matches the python version defined in the setup.cfg as done here. This way in future python upgrades, we won't have to modify the workflow files.
|
@harshavemula-ua @Vishak-V can we close this PR? I don't think we will merge this with DataStreamCLI and ForcingProcessor moving. |
|
@JordanLaserGit This update looks fine from my side. I’ve reviewed the changes and confirmed that none of my workflows or ongoing work are hampered by this PR. The adjustments to Python 3.10 and uv in the workflows don’t introduce conflicts with the areas I’m working on, and everything continues to build and run as expected in my scope. From my end, there are no blockers to proceeding with this change. ✅ |
|
The repositories have already been split. Changes made here won't be carried over. |
|
@Vishak-V has already begun moving this into forcingprocessor, just need to do the same now for datastreamcli. CIROH-UA/forcingprocessor#15 |
This PR updates the python version for the forcingprocessor to Python3.10 and uses uv for the workflows
Additions
Removals
Changes
Changed setup to use python 3.10 and update workflow accordingly
Testing
Screenshots
Notes
Todos
Checklist
Testing checklist
Target Environment support
Accessibility
Other