-
Notifications
You must be signed in to change notification settings - Fork 22
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
update supported Python version #356
Conversation
@mitchelbaker-cisa what implications does this PR have for the smoke test? |
Assuming pandas 2.x compatibility with Python 3.9+, there won't be any issues. However, the smoke test does uninstall numpy/install numpy v1.26.4 which should be removed if #326 is resolved by this PR. This also incentives me to implement #340 so we can specify Python versions to test against. |
Still need to do more testing, but the #326 should be resolved by this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Alden's comments, I think make those changes and it should be good to go.
Quick update. I'm testing with the latest changes. |
@aormu Not sure why this wasn't a problem before, but this appears to stem from the fact that types is an existing Python module and conflicts with our |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a bug, see the comment about circular dependencies (#356 (comment)).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing performed:
- On a Windows: install in a venv using the
python3 -m pip install .
Then run ScubaGoggles using thepython3 scuba.py gws...
command - On a Windows: install in a venv using the
python3 -m pip install .
Then run ScubaGoggles using thescubagoggles gws...
command - On a Windows: install in a venv using the
pip3 install -r requirements.txt
Then run ScubaGoggles using thepython3 scuba.py gws...
command - On a Linux: install in a venv using the
python3 -m pip install .
Then run ScubaGoggles using thepython3 scuba.py gws...
command - On a Linux: install in a venv using the
python3 -m pip install .
Then run ScubaGoggles using thescubagoggles gws...
command
Testing still needed on mac (@LaurenBassett I believe you can help with this). But as far as I can see, everything works as expected. @aormu I will approve once you address my remaining comment about the documentation.
Co-authored-by: Alden Hilton <106177711+adhilto@users.noreply.github.com>
Finished testing on Mac: Overall seeing the same results on Mac as on other OS. |
Not a circular import error :( Do you have the latest code? This commit fixed the circular import error on Windows. |
|
I thought I pulled the most recent when I had it. I can try again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed there's no longer a circular import on my Mac, going to go ahead and merge.
🗣 Description
Refresh Python dependencies.
💭 Motivation and context
Closes #321
Closes #326
🧪 Testing
Updated setup.py and requirements.txt and installed Python 3.10 locally on macOS. Run ScubaGoggles to validate there are no errors.
✅ Pre-approval checklist
✅ Pre-merge Checklist
Squash and merge
button.✅ Post-merge Checklist