-
Notifications
You must be signed in to change notification settings - Fork 277
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
maint: merge dependency specifications into setup.cfg #3236
maint: merge dependency specifications into setup.cfg #3236
Conversation
15e972b
to
0eace7e
Compare
0eace7e
to
fcb095a
Compare
Since this isn't going to get merged for at least a couple of days, I'll take the opportunity to experiment a little bit further than I originally intended |
c1c3d4a
to
5b033c6
Compare
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.
This looks like an improvement to me. Anyone know if this will impact things like using repo2docker
etc?
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 very happy about this! You have my green light once the tests pass
$CYTHON $NUMPY $CARTOPY $H5PY $MATPLOTLIB $SCIPY | ||
# windows_conda_requirements.txt is a survivance of test_requirements.txt | ||
# keep in sync: setup.cfg | ||
while read requirement; do conda install --yes $requirement; done < tests/windows_conda_requirements.txt |
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.
while read requirement; do conda install --yes $requirement; done < tests/windows_conda_requirements.txt | |
conda install --yes --file tests/windows_conda_requirements.txt |
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 don't think it works unfortunately. To the best of my knowledge, Conda can only install an named environment from a yaml file, and I didn't want to invest any more effort in a temporary band aid
# unyt so we'll match it here. | ||
python -m pip install numpy==1.13.3 cython==0.26.1 | ||
python -m pip install -r tests/test_minimal_requirements.txt | ||
python -m pip install -e .[test,minimal] |
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.
So much better!!
@neutrinoceros the errors on Jenkins is because dependencies are still installed by looking for the |
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.
self review to correct typos
Thanks @Xarthisius, looks like it worked for the answer test job, and likely for the py38 job as well. The docs and py38_unitests failed however. If there's anything I can do on this size to fix it let me know ! |
It's just me being clumsy. It will probably take a couple of iterations until I get it right... |
@yt-fido test this please |
BTW there are extra dependencies for docs that could be added to the setup:
I don't remember specific bugs that resulted in pinning Sphinx and nbconvert, but they're there for a reason I'm sure. |
It took a lot more iterations to me, I get it :)
Good to know, I'll make sure to include this (back in a moment). |
@Xarthisius does this work for you ? |
LGTM. Thanks! |
@neutrinoceros |
ah well, that's a good reason :) |
I really don't get the what's happening in the one test that still breaks on Jenkins. Also I think your setup for the docs job is likely ok now, but we ran into a timeout two times in a row... (third time's the charm ?) @yt-fido test this please |
Quick note that although the remaining failure happens consistently, the quantitative error that's produced fluctuates. |
@yt-fido test this please |
da8735f
to
e99429c
Compare
e99429c
to
ec04cdb
Compare
ec04cdb
to
c30980f
Compare
@yt-fido test this please |
@Xarthisius I just performed some diffs between a reference run and the latest one here for the failing docs job on Jenkins and an important difference is that on this branch, we apparently install yt twice, as the log contains python -m pip install '.[test,full,doc]'
...
python -m pip install -e '.[test,full,doc]' while in the ref run, it looks to me that yt itself is never installed but is only importable because we run python setup.py build_ext -i -f and every test is run from the top level of the repo. |
Doc build failed due to timeout. It happens from time to time when sphinx subthread dies for some reason. The build is kinda brittle this way. I'd give another chance (which I did a while ago). |
oh I see now that it's hanging again against the same volume rendering recipe that it did earlier today. The timeout already seemed to happen consistently on this branch. |
The errors I'm seeing in the log (that are causing the timeout):
|
How about rebasing onto main if you haven't already? |
that what I did a few hours ago |
This errors you mentioned also happen on a successful run. See for instance: https://tests.yt-project.org/job/yt_docs_new/1508/consoleFull |
The reason it fails is this:
we get that error if |
f39634e
to
1b4411d
Compare
1b4411d
to
7613a03
Compare
@yt-fido test this please |
Hurray ! Many thanks to @Xarthisius for your help with this one ! |
PR Summary
This is a second step forward in trying to solve #3074, following #3078.
Goal: remove as much content as possible from
tests/test_minimal_requirements.txt
tests/test_requirements.txt
tests/test_prerequirements.txt
and migrate it to setup.cfg
The migration process is error prone and I expect a few iterations will be required.
TODO
test
targetminimal
targetmatplotlib>=2.0.2
withmatplotlib==2.0.2
, do we actually satisfy that last requirement ?)full
target (corresponding totest_requirements.txt
pyyaml
VSpyaml
(both are currently "required" from test_requirements.txt but I bet that's a useless redundancy)This last step can be performed with a conditional similar to
Out of scope, followups
Cleanup yaml deps.
Currently minimal build requires
pyyaml
, but we usepyaml
(which itself depends onpyyaml
) in the other test envs. Note that both import asyaml
... Furtermore,yaml
is a soft dependency: part ofon_demand_imports
, tests use@requires_module("yaml")
), but there's actually one place in the code base that we import it without going through on_demand_imports.py :yt/yt/utilities/mesh_code_generation.py
Line 1 in 16cf684
so I'd be inclined to think that it's de-facto already a hard dependency ?
Cleanup pre-existing env targets ?
see
yt/setup.cfg
Line 61 in 16cf684
apparently the
hub
target only adds one requirement (girder_hub
) and is only used in parts of the command line tool. I'm not sure atm wether this is still used or tested for.Same goes for the
bottle
target.update : according to Kacper the first one is still used.
Remove
tests/windows_conda_requirements.txt
I introduced this file in the present PR as an intermediate step, though I didn't manage to suppress it completely yet.
Remove
tests/cartopy_requirements.txt
This is a convenience to mitigate problems with cartopy+shapely distribution. I hope it can be removed in the future, but that will probably require upstream changes regarding packaging.