-
Notifications
You must be signed in to change notification settings - Fork 326
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
DEV - Refactor CI and environment setup #1759
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.
I'm on board with this. I've left a few comments/questions on things that looked odd to me; not sure if they're mistakes or just showing my lack of familiarity with tox / actions / etc.
.github/workflows/CI.yml
Outdated
# oldest Python version with the oldest Sphinx version | ||
- os: ubuntu-latest | ||
python-version: "3.9" | ||
sphinx-version: "61" |
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.
bit confusing, expected 6.1
. why not use the tr -d .
trick on sphinx version too?
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.
Honestly I cannot remember why I did this but can change to match the rest
Minor conflict with #1826 that added timeouts. I merge with main, but timeouts may needed to be readded to actions in the future. |
…ta-sphinx-theme into trallard/update-ci
Co-authored-by: Daniel McCloy <dan@mccloy.info>
…ta-sphinx-theme into trallard/update-ci
I have addressed all the review comments, so if CI is happy, we could merge @drammock Edit I broke stuff so will fix it |
I won't have a chance to look again until Tuesday at the earliest, so please feel free to merge. I don't need to look again.
Sent from Proton Mail Android
…-------- Original Message --------
On 5/23/24 15:00, Tania Allard wrote:
I have addressed all the review comments, so if CI is happy, we could merge ***@***.***(https://github.com/drammock)
—
Reply to this email directly, [view it on GitHub](#1759 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AAN2AUYNT4IJ7YJH55Z257TZDZDHVAVCNFSM6AAAAABFZMWHKSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRXHEZDKMRUGQ).
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
tox.ini
Outdated
# keep this in sync across all docs environments | ||
extras = {[testenv:docs-no-checks]extras} | ||
deps = | ||
py39-sphinx61-docs: {py39-sphinx61-tests[deps]} |
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.
py39-sphinx61-docs: {py39-sphinx61-tests[deps]} | |
py39-sphinx61-docs: py39-sphinx61-tests[deps] |
I think this is the issue as the error message is
/opt/hostedtoolcache/Python/3.9.19/x64/bin/uv pip install './{py39-sphinx61-tests[deps]}'
error: Failed to read from the distribution cache
Caused by: failed to query metadata of file `/home/runner/work/pydata-sphinx-theme/pydata-sphinx-theme/{py39-sphinx61-tests[deps]}`
Thanks for all your help, @Carreau 🟣 - I fixed the issue with the deps, and the CI worked well. |
…#1800) * Add env variable to control test-them build location. I needed this to debug some things, so why not send it as a PR. See pydata#1798, maybe this should be folded in pydata#1759 ? * Update tests/conftest.py Co-authored-by: Daniel McCloy <dan@mccloy.info> --------- Co-authored-by: Daniel McCloy <dan@mccloy.info>
This PR addresses some of the concerns/issues raised in pydata#1292
This PR addresses some of the concerns/issues raised in #1292, more details, caveats, and TODOs below.
What is in this PR
tests_tox.yml
file; if accepted, this should completely replace the existingtests.yml
, but I thought I would leave it here in the interim so folks could compare as the CI gets executed. Some notable items:action
that performs that essential setup task and avoids duplicationnox
using bash scripts to install things, callingsphinx-build
directly, etc. Here, we usetox
for all the jobs and steps, providing syntax and tool consistency and removing cognitive overload for maintainers and contributors.macos-14
to our testing matrices; even with that and other combinations of OSes, python and Sphinx versions, even with these changes this CI suite is between 3.5 and 4 minutes faster than the existing onetox.ini
, which is the base of this refactor - this would meantox
will effectively replacenox
; therefore, thenoxfile.py
would be removed. The reasons why I am proposing this replacement are:tox
is faster thannox
, and I have found that theshould_install
step in thenoxfile
is flaky, and I have had to nuke my env multiple times.tox
is also very good at handling rebuilds and keeping track of dependencies changes which is something we have been struggling with usingnox
tox.ini
configuration, we can install PST indevelopment
mode for tasks like testing, profiling, etc. and perform other functions by installing a packaged version of PST; this is mostly for building our docs, which more closely mimics how any other user would use PST, thus helping with testing and validation on that end tootox
for both CI, local development, docs, compiling, lining, and all (actually, we drop the pre-commit action dependency here and do our lint)What it does not do
stb
andsphinx-build
as they serve different purposes -sbt
helps with the compilation of assets and all node-related things as well as packaging, whilesphinx-build
only generates our docs in HTML format. So, we are keeping these two dependencies as we need both.Still to do
github_actions_install.sh
nox
-> deferred to other PRnox
Deferred to other PR
To move this PR and facilitate the merge, I have decided to move this other significant amount of work to another PR.
Note that this means this PR does not get rid of
nox
, so until this follow-up PR, we havenox
andtox
, but I have started to move the documentation here to include the use oftox
for development tasks and the like.noxfile.py
docs-live
set-up, as noted in can we simplify how our tests are defined and run? #1292 This is extremely slow (Sphinx tho), so I am not sure if I can make any improvements. But it can be so slow that I cannot use it or decide not to use it.Questions
readthedocs.yml
) and Python 3.12 by default for some of our docs building tasks. Do we want to update the version in RTD for consistency?If you made it all the way here you deserve a cookie 🍪, thank you 💜