CI: clean environment.yml, comment that req-dev is generated #30392
Conversation
| @@ -33,7 +33,8 @@ dependencies: | |||
| - nbconvert>=5.4.1 | |||
| - nbsphinx | |||
| - pandoc | |||
| # Dask and its dependencies | |||
|
|
|||
| # Dask and its dependencies (that dont install with dask) | |||
There was a problem hiding this comment.
IIUC conda install dask does the right thing without needing to specify everything else, but pip install dask does not. could we unify all this as dask[dataframe]? also i guess put it in the "downstream tests" block?
There was a problem hiding this comment.
@TomAugspurger might have ideas
There was a problem hiding this comment.
dask[dataframe] works for installing from PyPI, but there isn't a dask[dataframe] or dask-dataframe conda package.
|
|
||
| pip_content = "\n".join(pip_deps) | ||
| fname = os.path.split(conda_fname)[1] |
There was a problem hiding this comment.
Is there a reason for the split here?
There was a problem hiding this comment.
otherwise we get a full path including username
| @@ -33,7 +33,8 @@ dependencies: | |||
| - nbconvert>=5.4.1 | |||
| - nbsphinx | |||
| - pandoc | |||
| # Dask and its dependencies | |||
|
|
|||
| # Dask and its dependencies (that dont install with dask) | |||
There was a problem hiding this comment.
@TomAugspurger might have ideas
| header = ( | ||
| f"# This file is auto-generated from {fname}, do not modify.\n" | ||
| "# See that file for comments about the need/usage of each depdendency.\n\n" | ||
| ) | ||
| pip_content = header + "\n".join(pip_deps) |
There was a problem hiding this comment.
Small personal preference, but I think this would be simpler if when defining pip_deps above, instead of pip_content = [] you write:
pip_content = [
f"# This file is auto-generated from {fname}, do not modify.",
"# See that file for comments about the need/usage of each depdendency.",
]If you do that, all this can be deleted here (you'll have to define fname before).
There was a problem hiding this comment.
ATM the pip_deps list has a specific meaning. id prefer to retain that
There was a problem hiding this comment.
Not important, but headers could still be a list, and use "\n".join(headers + pip_deps). I think it'll still be an improvement, avoiding the \n in the strings. Just saying, happy to get this merge as is.
There was a problem hiding this comment.
I would actually write this as a triple quoted string.
header = f"""
...
..."""
which avoids the embedded newlines
There was a problem hiding this comment.
triple quoted strings that arent at the module-level mess with code-folding, so i prefer to avoid them
There was a problem hiding this comment.
k that's what i would have done, but ok as is.
There is a little more id like to do in environment.yml to put docbuild things in the same place, but holding off to keep the diff tidier. Also it appears there are some unused things in environment.yml that we can look into removing, separate PR