-
Notifications
You must be signed in to change notification settings - Fork 24
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
Drop Python 3.7 support and fix test workflows #339
Conversation
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.
LGTM! Just some minor questions for additional context
.github/workflows/test-with-pip.yml
Outdated
if: matrix.os != 'macos-latest' | ||
- name: Install dependencies and local packages (macOS) | ||
run: python -m pip install .[preferences] | ||
# tables currently won't build on Apple Silicon |
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.
Just for context, I presume our CI tests don't require tables
to run, or are the ones that do segregated into a h5
-only set?
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.
Ah, it's the latter I see, based on the test traceback in the macos-latest
CI job runs: skipped 'PyTables not available'
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.
Is it worth making an issue to track this incompatibility, or are we not promising support for cross-platform builds anyway?
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'm not sure whether it's really worth an issue here - I don't see that there's anything actionable for apptools other than wait until PyTables figures out how to support Apple Silicon. The apptools code should all work the moment that that happens.
The most relevant upstream issue is here: PyTables/PyTables#1165
I should probably at least link to that in the workflow.
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.
Ah, it's the latter I see
Right: PyTables is in effect an optional dependency. Many of our uses of apptools only care about some other part (e.g., preferences) and don't care about the persistence pieces.
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'm not sure whether it's really worth an issue here
Okay, I take that back - there is an action to be taken, which is to remove the workaround from our workflows. I'll open issues both for this and for the NumPy 2.0 incompatibility.
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.
"enthought_sphinx_theme", | ||
"flake8", | ||
"flake8_ets", | ||
"sphinx", |
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.
Again, just for context, I see that we don't have any CI jobs that build documentation, so are we using Sphinx anywhere?
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.
We use Sphinx to build docs after making a release, and the current workflow for that is to do it manually via etstool.
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.
Agreed that we should ideally have a workflow for that.
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.
Got it, thanks!
This PR brings our CI jobs back to working state, and extends them to cover more recent Python versions:
etstool.py
has been updated to support Python 3.11test-with-pip.yml
workflow has dropped 3.7 and added 3.12test-with-edm.yml
workflow has dropped 3.6 and added 3.11dependabot.yml
config has been added, just for GitHub ActionsChecklist