Skip to content
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

[ci] raise floors on CI dependencies #6375

Merged
merged 21 commits into from
Apr 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .ci/conda-envs/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# conda-envs

This directory contains files used to create `conda` environments for development
and testing of LightGBM.

The `.txt` files here are intended to be used with `conda create --file`.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really convinced why a .txt file would be more appropriate than a .yml file here 🤔 I've been happily using .yml files for environment creation for years although I've been using micromamba instead of conda.

While I think that the former would be more fitting for use in CI in general, does anything prevent us from using conda env create -f <file>.yml? I'm almost certain that this works.

Copy link
Collaborator

@jmoralez jmoralez Apr 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've also used micromamba install on the base env (micromamba doesn't have a base env so we could keep the current name) which allows you to specify the python version and a yaml file for the dependencies (example)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does anything prevent us from using conda env create -f .yml?

I described this in "Why are these .txt files and not conda .yaml files?" in the PR description. I'll add more details here.

I wanted to be able to pass in a Python constraint from the command line, to avoid these alternatives I could think of for constraining Python version:

  • using templated statements like python=${PYTHON_VERSION} in an env.yml file and needing to involve some templater like envsubst (docs) to use them
  • generating individual constraint for each Python version LightGBM tests against, each containing a dependency like python=3.10.*
  • continuing to have these lists of dependency requirements hard-coded in CI scripts

As of the latest conda (v24.3.0), conda env create does not support mixing files and command-line constraints.

Given a file env.yaml with these contents:

channels:
  - nodefaults
  - conda-forge
dependencies:
  - scipy>=1.0

This

conda create \
   --name delete-me \
   --file ./env.yaml

Yields this

CondaValueError: could not parse '- nodefaults' in: ./env.yaml

conda env create can read files like that... but you can't pass additional constraints from the command line.

conda env create \
   --name delete-me \
   --file ./env.yaml \
   python=3.10
SpecNotFound: Invalid name 'python=3.10', try the format: user/package

I looked some more into this tonight and found this relevant issue with discussion about changes to these APIs in conda:

micromamba install ... allows you to specify the python version and a yaml file for the dependencies

Oh cool! I didn't realize micromamba allowed for that. That behavior looks like exactly what I'm trying to get here.

however ... I don't support switching to micromamba here in LightGBM's CI. It's described as a "reimplementation" of conda / mamba in its docs (docs link). Part of the motivation of moving to environment files in this PR is to help make the development of LightGBM easier. I'd really like to avoid having that experience start with "install this specific other conda alternative". I also really would not be excited about the prospect of rebuilding the images from https://github.com/guolinke/lightgbm-ci-docker to include micromamba.

Especially just for the benefit of "use .yml files because .txt files seem weird".

@jmoralez @borchero can you think of any other functional reasons that what I've proposed in this PR (using conda create + .txt files + python=${PYTHON_VERSION} from the command line) is problematic?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just don't think txt files are widely adopted in the conda commands. For example, suppose someone already has an environment with LightGBM in it and just wants to add the dependencies to develop, they could just use conda/mamba env update -f env.yaml. conda env update also allows passing multiple files, so if we end up having more than one file they could be passed in that single command.

The two step creation (python version first and dependencies second) isn't really problematic (I think that's what most people do) and I don't think it slows down the second resolution.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my experience, it absolutely does speed up the total time-to-environment-ready to do everything in one step instead of creating an environment and then updating it: #5743.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you support still preserving the "create environment in all one shot" approach by having one YAML file per Python version, like this?

I think that's reasonable, we're extensively using this mechanism at our company 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

With the constraints "create the environment with all dependencies in a single command" and "keep using conda / mamba", there are 2 approaches we've identified:

  1. use conda create --file .ci/ci-core.txt python=${PYTHON_VERSION}
  2. create one YAML file per combination and use something like conda env create --file ./.ci/conda-envs/ci-core-py${PYTHON_VERSION}.yml

If I'm reading this correctly, @jmoralez prefers Option 1 and you prefer Option 2.

I also slightly prefer Option 1, at least right now where all not-Python-3.7 environments can share the same dependency constraints.

If we were to have more Python-version-specific lists of dependencies, I think I'd prefer Option 2 more.

@borchero is it alright with you if we move forward with the Option 1 approach right now and see how it goes? Since this is just affecting CI and commands that we document in a README, I think it should be low-risk to be wrong and end up changing this pattern in the future.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, fine for me, please move ahead with option 1 to improve the CI setup :)

On a slightly related note, this PR prompted me to think about the use of lock files... this would ensure entirely reproducible environments (i.e. fewer CI failures) and would essentially fully alleviate the need for solving in CI jobs. Potentially something for a future PR... 😄

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thanks very much! I'll get this PR building and and merge it on @jmoralez 's existing approval of the current approach.

I think we're almost immediately gonna be talking about this again when we decide how far to go in "dropping" Python 3.8 support like discussed further up in this PR.

the use of lock files... this would ensure entirely reproducible environments

I'd support using lock files (or just generally == types constraints) for the jobs using Python versions that are using end-of-life Python versions or operating systems.

But for the others, I think it's valuable to let dependencies float when we can. That means that LightGBM is continually tested against new releases of all its dependencies, which more evenly spreads out the work of reacting to breaking changes over time, and reduces the total amount of effort required.

For example, I'm thinking about cases like this:

Because CI broke here shortly after a new dask release came out, there was a fairly small changeset in dask to investigate to try to find the root cause. If instead we use == pins here and then only hit that error say a month or 2 months later, the debugging effort would have been much higher. And we would have missed the opportunity to get in a fix like dask/dask#11007 quickly to limit how many versions of the dependency exhibit the behavior that's problematic for lightgbm .

Letting things float also more closely makes CI match the real-world experience of someone starting a new project and running pip install lightgbm scikit-learn numpy or something, where they'd get the latest versions.

Since this project publishes a Python library used in a lot of different contexts, not an application that's distributed as a binary or container image, I think it's valuable to continue that practice of letting the dependencies somewhat float + regularly bumping up the floors to continue getting fast environment solves.

All that said though... if you disagree or see some other opportunities to improve the way we get dependencies in CI, I definitely encourage you to write up a proposal in an issue! We have a loosely-enforced convention of doing those in issues tagged [RFC] ("request for comment"). You can see some at https://github.com/microsoft/LightGBM/issues?q=%22%5BRFC%5D%22+is%3Aissue+is%3Aclosed.

Thanks to both of you for talking through all this with me, it's good that we keep challenging the current state of how things are set up.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I didn't know about the [RFC]-type issues yet, I'll think about it a bit more (taking into account your comment, thanks for the elaboration ;) and might come back to this then 😄


For details on that, see the `conda` docs:

* `conda create` docs ([link](https://conda.io/projects/conda/en/latest/commands/create.html))
* "Managing Environments" ([link](https://conda.io/projects/conda/en/latest/user-guide/tasks/manage-environments.html))
56 changes: 56 additions & 0 deletions .ci/conda-envs/ci-core-py37.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
# [description]
#
# Similar to ci-core.txt, but specific to Python 3.7.
#
# Unlike ci-core.txt, this includes a Python version and uses
# `=` and `<=` pins to make solves faster and prevent against
# issues like https://github.com/microsoft/LightGBM/pull/6370.
#
# [usage]
#
# conda create \
# --name test-env \
# --file ./.ci/conda-envs/ci-core-py37.txt
#

# python
python=3.7.*

# direct imports
cffi=1.15.*
# older versions of Dask are incompatible with pandas>=2.0, but not all conda packages' metadata accurately reflects that
#
# ref: https://github.com/microsoft/LightGBM/issues/6030
dask=2022.2.*
distributed=2022.2.*
joblib=1.3.*
matplotlib-base=3.5.*
numpy=1.21.*
pandas=1.3.*
pyarrow=9.0.*
# python-graphviz 0.20.2 is not compatible with Python 3.7
# ref: https://github.com/microsoft/LightGBM/pull/6370
python-graphviz=0.20.1
scikit-learn=1.0.*
scipy=1.7.*

# testing-only dependencies
cloudpickle=2.2.*
pluggy=1.0.*
psutil=5.9.3
pytest=7.4.*

# other recursive dependencies, just
# pinned here to help speed up solves
bokeh=2.4.*
fsspec=2023.1.*
msgpack-python=1.0.*
pluggy=1.0.*
pytz=2024.1
setuptools=59.8.*
snappy=1.1.*
tomli=2.0.*
tornado=6.1.*
wheel=0.42.*
zict=2.2.*
zipp=3.15.*
40 changes: 40 additions & 0 deletions .ci/conda-envs/ci-core.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
# [description]
#
# Core dependencies used across most LightGBM continuous integration (CI) jobs.
#
# 'python' constraint is intentionally omitted, so this file can be reused across
# Python versions.
#
# These floors are not the oldest versions LightGBM supports... they're here just to make conda
# solves faster, and should generally be the latest versions that work for all CI jobs using this.
#
# [usage]
#
# conda create \
# --name test-env \
# --file ./.ci/conda-envs/ci-core.txt \
# python=3.10
#

# direct imports
cffi>=1.16
dask>=2023.5.0
joblib>=1.3.2
matplotlib-base>=3.7.3
numpy>=1.24.4
pandas>2.0
pyarrow>=6.0
python-graphviz>=0.20.3
scikit-learn>=1.3.2
scipy>=1.1

# testing-only dependencies
cloudpickle>=3.0.0
psutil>=5.9.8
pytest>=8.1.1

# other recursive dependencies, just
# pinned here to help speed up solves
pluggy>=1.4.0
setuptools>=69.2
wheel>=0.43
41 changes: 16 additions & 25 deletions .ci/test.sh
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ if [[ "$TASK" == "cpp-tests" ]]; then
exit 0
fi

# including python=version[build=*cpython] to ensure that conda doesn't fall back to pypy
CONDA_PYTHON_REQUIREMENT="python=$PYTHON_VERSION[build=*cpython]"

if [[ $TASK == "if-else" ]]; then
Expand Down Expand Up @@ -82,10 +83,10 @@ if [[ $TASK == "lint" ]]; then
${CONDA_PYTHON_REQUIREMENT} \
'cmakelint>=1.4.2' \
'cpplint>=1.6.0' \
'matplotlib>=3.8.3' \
'matplotlib-base>=3.8.3' \
'mypy>=1.8.0' \
'pre-commit>=3.6.0' \
'pyarrow>=14.0' \
'pyarrow>=6.0' \
'r-lintr>=3.1'
source activate $CONDA_ENV
echo "Linting Python code"
Expand Down Expand Up @@ -131,28 +132,18 @@ if [[ $TASK == "check-docs" ]] || [[ $TASK == "check-links" ]]; then
exit 0
fi

# older versions of Dask are incompatible with pandas>=2.0, but not all conda packages' metadata accurately reflects that
#
# ref: https://github.com/microsoft/LightGBM/issues/6030
CONSTRAINED_DEPENDENCIES="'dask>=2023.5.0' 'distributed>=2023.5.0' 'pandas>=2.0' python-graphviz"
if [[ $PYTHON_VERSION == "3.7" ]]; then
CONSTRAINED_DEPENDENCIES="'dask' 'distributed' 'python-graphviz<0.20.2' 'pandas<2.0'"
CONDA_REQUIREMENT_FILES="--file ${BUILD_DIRECTORY}/.ci/conda-envs/ci-core-py37.txt"
else
CONDA_REQUIREMENT_FILES="--file ${BUILD_DIRECTORY}/.ci/conda-envs/ci-core.txt"
fi

# including python=version[build=*cpython] to ensure that conda doesn't fall back to pypy
mamba create -q -y -n $CONDA_ENV \
${CONSTRAINED_DEPENDENCIES} \
cffi \
cloudpickle \
joblib \
matplotlib \
numpy \
psutil \
pyarrow \
pytest \
mamba create \
-y \
-n $CONDA_ENV \
${CONDA_REQUIREMENT_FILES} \
${CONDA_PYTHON_REQUIREMENT} \
scikit-learn \
scipy || exit 1
|| exit 1

source activate $CONDA_ENV

Expand Down Expand Up @@ -314,10 +305,10 @@ matplotlib.use\(\"Agg\"\)\
' plot_example.py # prevent interactive window mode
sed -i'.bak' 's/graph.render(view=True)/graph.render(view=False)/' plot_example.py
# requirements for examples
mamba install -q -y -n $CONDA_ENV \
h5py \
ipywidgets \
notebook
mamba install -y -n $CONDA_ENV \
'h5py>=3.10' \
'ipywidgets>=8.1.2' \
'notebook>=7.1.2'
for f in *.py **/*.py; do python $f || exit 1; done # run all examples
cd $BUILD_DIRECTORY/examples/python-guide/notebooks
sed -i'.bak' 's/INTERACTIVE = False/assert False, \\"Interactive mode disabled\\"/' interactive_plot_example.ipynb
Expand All @@ -329,7 +320,7 @@ matplotlib.use\(\"Agg\"\)\
dask \
distributed \
joblib \
matplotlib \
matplotlib-base \
psutil \
pyarrow \
python-graphviz \
Expand Down
30 changes: 14 additions & 16 deletions .ci/test_windows.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -46,23 +46,21 @@ conda config --set always_yes yes --set changeps1 no
# ref:
# * https://stackoverflow.com/a/62897729/3986677
# * https://github.com/microsoft/LightGBM/issues/5899
conda install brotlipy
conda install "brotlipy>=0.7"

conda update -q -y conda
conda create -q -y -n $env:CONDA_ENV `
cffi `
cloudpickle `
joblib `
matplotlib `
numpy `
pandas `
psutil `
pyarrow `
pytest `
"python=$env:PYTHON_VERSION[build=*cpython]" `
python-graphviz `
scikit-learn `
scipy ; Check-Output $?

if ($env:PYTHON_VERSION -eq "3.7") {
$env:CONDA_REQUIREMENT_FILE = "$env:BUILD_SOURCESDIRECTORY/.ci/conda-envs/ci-core-py37.txt"
} else {
$env:CONDA_REQUIREMENT_FILE = "$env:BUILD_SOURCESDIRECTORY/.ci/conda-envs/ci-core.txt"
}

conda create `
-y `
-n $env:CONDA_ENV `
--file $env:CONDA_REQUIREMENT_FILE `
"python=$env:PYTHON_VERSION[build=*cpython]" ; Check-Output $?

if ($env:TASK -ne "bdist") {
conda activate $env:CONDA_ENV
Expand Down Expand Up @@ -124,7 +122,7 @@ if (($env:TASK -eq "regular") -or (($env:APPVEYOR -eq "true") -and ($env:TASK -e
cd $env:BUILD_SOURCESDIRECTORY/examples/python-guide
@("import matplotlib", "matplotlib.use('Agg')") + (Get-Content "plot_example.py") | Set-Content "plot_example.py"
(Get-Content "plot_example.py").replace('graph.render(view=True)', 'graph.render(view=False)') | Set-Content "plot_example.py" # prevent interactive window mode
conda install -q -y -n $env:CONDA_ENV "h5py>3.0" ipywidgets notebook
conda install -y -n $env:CONDA_ENV "h5py>=3.10" "ipywidgets>=8.1.2" "notebook>=7.1.2"
foreach ($file in @(Get-ChildItem *.py)) {
@("import sys, warnings", "warnings.showwarning = lambda message, category, filename, lineno, file=None, line=None: sys.stdout.write(warnings.formatwarning(message, category, filename, lineno, line))") + (Get-Content $file) | Set-Content $file
python $file ; Check-Output $?
Expand Down
Loading