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.

Sign up for GitHub

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

Jump to bottom

Add needed pip packages to src/environment.yml, minimize conda environments #35593

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from
Open

Add needed pip packages to src/environment.yml, minimize conda environments #35593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
65277dc
Remove conda.txt for mathics, pari_jupyter
mkoeppe May 1, 2023
e18a7ab
build/pkgs/jupymake: Omit from src/environment-optional.yml
mkoeppe May 1, 2023
addd6e4
build/pkgs/pari_jupyter: Restore conda.txt
mkoeppe May 1, 2023
346b16d
build/pkgs/jupymake/distros/conda.txt: Fix up
mkoeppe May 2, 2023
d46bfa9
build/pkgs: Add some more conda.txt
mkoeppe May 3, 2023
0da1ac2
build/pkgs/jupyterlab_widgets/distros/conda.txt: Fix up
mkoeppe May 3, 2023
aafebf4
build/pkgs/pycygwin: Do not install on conda
mkoeppe May 3, 2023
3fb54ff
build/pkgs: Add missing conda.txt
mkoeppe Sep 27, 2023
22ba07b
bootstrap-conda: Minimize environments
mkoeppe Oct 3, 2023
fcb537d
build/pkgs: Make boost_cropped an order-only dep (it's header-only)
mkoeppe Dec 19, 2023
38c8c2c
bootstrap-conda: Add pip lines also in src/environment.yml
mkoeppe Jun 22, 2024
db482e2
build/pkgs/cliquer/distros/conda.txt: Add lower bound
mkoeppe Nov 25, 2023
8b8c45e
build/pkgs/{ecl,gc,libatomic_ops}: Add version bounds for conda
mkoeppe Nov 25, 2023
5af6f07
pkgs/sage-conf_conda/setup.py: Do not use force for packages that may…
mkoeppe Nov 25, 2023
a5e4c8f
pkgs/sage-conf_conda/setup.py: Use --enable-system-sitepackages
mkoeppe Nov 25, 2023
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
47 changes: 43 additions & 4 deletions bootstrap-conda
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ OPTIONAL_PACKAGES=
SAGELIB_PACKAGES=
SAGELIB_OPTIONAL_PACKAGES=
DEVELOP_PACKAGES=
UNCONSTRAINED_PACKAGES=
CONSTRAINED_PACKAGES=

eval $(sage-package properties --format=shell :all:)

Expand All @@ -25,6 +27,15 @@ for PKG_BASE in $(sage-package list --has-file distros/conda.txt --exclude _sage
SYSTEM_PACKAGES_FILE=$PKG_SCRIPTS/distros/conda.txt
PKG_SYSTEM_PACKAGES=$(echo $(${STRIP_COMMENTS} $SYSTEM_PACKAGES_FILE))
if [ -n "$PKG_SYSTEM_PACKAGES" ]; then
case "$PKG_SYSTEM_PACKAGES" in
*([-A-Za-z0-9_ ])) # just a package name
UNCONSTRAINED_PACKAGES+=" $PKG_BASE"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's a fragile design to use "unconstrained version" as an indicator for including or excluding a certain dependency in the environment. It would prevent us from adding a version constraint to non-direct dependencies (although I have to admit I don't even understand the purpose of having conda.txt files for such dependencies) and forces us to specify a version constraint for those packages that do should end up.
Going with Python's 'explicit is better than implicit' slogan, I think it would be better to have a certain indicator file that says 'I'm a direct dependency, so include me please'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would prevent us from adding a version constraint to non-direct dependencies

To the contrary. It does the right thing -- namely including the dependency with the version constraint in the environment file -- for the non-direct dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

design to use "unconstrained version" as an indicator for including or excluding a certain dependency in the environment.

That's not what it does. Packages are excluded by the minimizer from the environment file when they are implied by dependence. But we never exclude packages that have their own version constraints, so that the version constraint is not lost.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need for any of the version constraints you added in this PR (intrinsic need, as in "if you install an older version that it's known to fail"). Thus you use version constraints as a a signal to include the given package in the environment file. I think this is not a good use of version constraints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, there is an "intrinsic need" for these version constraints; they are the same version bounds that configure checks, each with good reasons.

It's just that "extrinsically" we have so far gotten away with not setting them -- simply because there is nothing that would force an older unsuitable version. But when users mix our environment with other conda packages, we can run into trouble. (And no, it's not better to wait for the bug report.)

Yes, I reuse this mechanism here for another purpose -- so that not another mechanism needs to be invented.

;;
*) # a package name with version constraints or other decorations;
# we never remove such packages when minimizing package lists
CONSTRAINED_PACKAGES+=" $PKG_BASE"
;;
esac
if [ -f $PKG_SCRIPTS/spkg-configure.m4 ]; then
if grep -q SAGE_PYTHON_PACKAGE_CHECK $PKG_SCRIPTS/spkg-configure.m4; then
# Python package that would need --enable-system-site-packages to be used
Expand Down Expand Up @@ -67,7 +78,34 @@ for PKG_BASE in $(sage-package list --has-file distros/conda.txt --exclude _sage
done
unset PKG_SYSTEM_PACKAGES

[ -n "$BOOTSTRAP_VERBOSE" ] && echo "## Collected:" && set | grep PACKAGES=
[ -n "$BOOTSTRAP_VERBOSE" ] && printf "##\n## Collected:\n##\n" && set | grep PACKAGES=

# Minimize

PACKAGES=$(sage-package list $(sage-package list --include-dependencies $PACKAGES --exclude $UNCONSTRAINED_PACKAGES) \
$(sage-package list --exclude-dependencies $PACKAGES))

BOOTSTRAP_PACKAGES=$(sage-package list $(sage-package list --include-dependencies $BOOTSTRAP_PACKAGES --exclude $UNCONSTRAINED_PACKAGES) \
$(sage-package list --exclude-dependencies $BOOTSTRAP_PACKAGES) \
--exclude $(sage-package list --include-dependencies $PACKAGES))

DEVELOP_PACKAGES=$(sage-package list $(sage-package list --include-dependencies $DEVELOP_PACKAGES --exclude $UNCONSTRAINED_PACKAGES) \
$(sage-package list --exclude-dependencies $DEVELOP_PACKAGES) \
--exclude $(sage-package list --include-dependencies $PACKAGES $BOOTSTRAP_PACKAGES))

OPTIONAL_PACKAGES=$(sage-package list $(sage-package list --include-dependencies $OPTIONAL_PACKAGES --exclude $UNCONSTRAINED_PACKAGES) \
$(sage-package list --exclude-dependencies $OPTIONAL_PACKAGES) \
--exclude $(sage-package list --include-dependencies $PACKAGES $BOOTSTRAP_PACKAGES))

SAGELIB_PACKAGES=$(sage-package list $(sage-package list --include-dependencies $SAGELIB_PACKAGES --exclude $UNCONSTRAINED_PACKAGES) \
$(sage-package list --exclude-dependencies $SAGELIB_PACKAGES) \
--exclude $(sage-package list --include-dependencies $PACKAGES $BOOTSTRAP_PACKAGES))

SAGELIB_OPTIONAL_PACKAGES=$(sage-package list $(sage-package list --include-dependencies $SAGELIB_OPTIONAL_PACKAGES --exclude $UNCONSTRAINED_PACKAGES) \
$(sage-package list --exclude-dependencies $SAGELIB_OPTIONAL_PACKAGES) \
--exclude $(sage-package list --include-dependencies $PACKAGES $BOOTSTRAP_PACKAGES $SAGELIB_PACKAGES $OPTIONAL_PACKAGES))

[ -n "$BOOTSTRAP_VERBOSE" ] && printf "##\n## Minimized:\n##\n" && set | grep PACKAGES=

# Translate to system packages
export ENABLE_SYSTEM_SITE_PACKAGES=yes # Disable filtering in sage-get-system-packages
Expand All @@ -79,7 +117,7 @@ SAGELIB_OPTIONAL_SYSTEM_PACKAGES=$(sage-get-system-packages conda $SAGELIB_OPTIO
DEVELOP_SYSTEM_PACKAGES=$(sage-get-system-packages conda $DEVELOP_PACKAGES)
unset ENABLE_SYSTEM_SITE_PACKAGES

[ -n "$BOOTSTRAP_VERBOSE" ] && echo "## Translated to system:" && set | grep SYSTEM_PACKAGES=
[ -n "$BOOTSTRAP_VERBOSE" ] && printf "##\n## Translated to system:\n##\n" && set | grep SYSTEM_PACKAGES=

echo >&2 $0:$LINENO: generate conda environment files

Expand Down Expand Up @@ -133,6 +171,7 @@ echo >&2 $0:$LINENO: generate conda environment files
(
echo >&4 " - pip:"
echo >&5 " - pip:"
echo >&6 " - pip:"
Copy link
Contributor

Choose a reason for hiding this comment

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

The standard environment file doesn't contain a pip section by design.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, if that's by design (whose?), then that's a severe bug in it, and this is the fix.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not there to check that all standard packages are installed via conda. If anything is a bug, then it's the presence of the pip section for optional and dev environments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not there to check that all standard packages are installed via conda.

No, it is not being checked by anything. sagenb_export is a standard package, and so far it has just been omitted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The bigger flaw is related to what we explained to you over in #36765 (comment):

Creating conda packages is a downstream activity. If we were to take the design idea that "all standard packages need to have a conda package" seriously, it would mean that we could never add a standard package in our capacity as the upstream project.

But neither is this current practice (neither with conda nor with any other downstream package repository), nor has such a change of policy been established or even discussed anywhere. And certainly this change cannot come into existence by gaslighting PR authors.

for PKG_BASE in $(sage-package list :standard: :optional: --has-file requirements.txt --no-file distros/conda.txt --no-file src; sage-package list :standard: :optional: --has-file version_requirements.txt --no-file requirements.txt --no-file distros/conda.txt --no-file src); do
eval PKG_SCRIPTS=\$path_$PKG_BASE PKG_TYPE=\$type_$PKG_BASE
SYSTEM_PACKAGES_FILE=$PKG_SCRIPTS/requirements.txt
Expand All @@ -144,15 +183,15 @@ echo >&2 $0:$LINENO: generate conda environment files
else
case "$PKG_BASE:$PKG_TYPE" in
$DEVELOP_SPKG_PATTERN:*) FD=4;;
*:standard) FD="4 5";;
*:standard) FD="4 5 6";;
*) FD=5;;
esac
${STRIP_COMMENTS} $SYSTEM_PACKAGES_FILE | while read -r line; do
[ -n "$line" ] && for fd in $FD; do echo >&$fd " - $line"; done
done
fi
done
) 4>> /dev/null 5>> src/environment-optional-template.yml
) 4>> /dev/null 5>> src/environment-optional-template.yml 6>>src/environment-template.yml

for f in environment environment-optional src/environment src/environment-optional src/environment-dev; do
for python_version in 3.9 3.10 3.11; do
Expand Down
1 change: 1 addition & 0 deletions build/pkgs/argon2_cffi_bindings/distros/conda.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
argon2-cffi-bindings
2 changes: 1 addition & 1 deletion build/pkgs/brial/dependencies
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
boost_cropped m4ri libpng | pkgconf
m4ri libpng | pkgconf boost_cropped

----------
All lines of this file are ignored except the first.
1 change: 1 addition & 0 deletions build/pkgs/cachetools/distros/conda.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
cachetools
mkoeppe marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions build/pkgs/calver/distros/conda.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
calver
1 change: 1 addition & 0 deletions build/pkgs/chardet/distros/conda.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
chardet
4 changes: 3 additions & 1 deletion build/pkgs/cliquer/distros/conda.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
cliquer
cliquer>=1.21
# We include the version constraint so that bootstrap-conda's environment minimizer
# does not eliminate it (as a declared dependency of the giac SPKG)
1 change: 1 addition & 0 deletions build/pkgs/colorama/distros/conda.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
colorama
1 change: 1 addition & 0 deletions build/pkgs/contourpy/distros/conda.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
contourpy
2 changes: 1 addition & 1 deletion build/pkgs/e_antic/dependencies
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
$(MP_LIBRARY) flint boost_cropped
$(MP_LIBRARY) flint | boost_cropped

----------
All lines of this file are ignored except the first.
2 changes: 1 addition & 1 deletion build/pkgs/ecl/distros/conda.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
ecl
ecl>=21.2.1
2 changes: 1 addition & 1 deletion build/pkgs/gc/distros/conda.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
bdw-gc
bdw-gc>=7.6.4
1 change: 1 addition & 0 deletions build/pkgs/hatch_fancy_pypi_readme/distros/conda.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
hatch-fancy-pypi-readme
1 change: 1 addition & 0 deletions build/pkgs/hatch_nodejs_version/distros/conda.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
hatch-nodejs-version
1 change: 1 addition & 0 deletions build/pkgs/hatch_vcs/distros/conda.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
hatch-vcs
1 change: 1 addition & 0 deletions build/pkgs/jupymake/distros/conda.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# The presence of this file ensures that we do not try to pip-install JuPyMake (there is no polymake conda package).
mkoeppe marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions build/pkgs/jupyterlab_widgets/distros/conda.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
jupyterlab_widgets
2 changes: 1 addition & 1 deletion build/pkgs/libatomic_ops/distros/conda.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
libatomic_ops
libatomic_ops>=7.6.2
1 change: 0 additions & 1 deletion build/pkgs/mathics/distros/conda.txt
View file
Open in desktop

This file was deleted.

1 change: 1 addition & 0 deletions build/pkgs/meson/distros/conda.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
meson
1 change: 1 addition & 0 deletions build/pkgs/meson_python/distros/conda.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
meson-python
2 changes: 1 addition & 1 deletion build/pkgs/papilo/dependencies
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
$(MP_LIBRARY) boost_cropped onetbb $(BLAS) gfortran | cmake
$(MP_LIBRARY) onetbb $(BLAS) gfortran | cmake boost_cropped
1 change: 1 addition & 0 deletions build/pkgs/pycygwin/distros/conda.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# The presence of this file ensures that we do not try to pip-install pycygwin -- which would fail with a compilation error
1 change: 1 addition & 0 deletions build/pkgs/pyproject_api/distros/conda.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
pyproject-api
1 change: 1 addition & 0 deletions build/pkgs/pyproject_metadata/distros/conda.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
pyproject-metadata
2 changes: 1 addition & 1 deletion build/pkgs/sagelib/dependencies
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
FORCE $(SCRIPTS) boost_cropped $(BLAS) brial cliquer cypari cysignals cython ecl eclib ecm flint libgd gap giac givaro glpk gmpy2 gsl iml importlib_metadata importlib_resources jupyter_core lcalc lrcalc_python libbraiding libhomfly libpng linbox m4ri m4rie memory_allocator mpc mpfi mpfr $(MP_LIBRARY) ntl numpy pari pip pkgconfig planarity ppl pplpy primesieve primecount primecountpy $(PYTHON) requests rw sage_conf singular symmetrica typing_extensions $(PCFILES) | $(PYTHON_TOOLCHAIN) sage_setup $(PYTHON) pythran
FORCE $(SCRIPTS) $(BLAS) brial cliquer cypari cysignals cython ecl eclib ecm flint libgd gap giac givaro glpk gmpy2 gsl iml importlib_metadata importlib_resources jupyter_core lcalc lrcalc_python libbraiding libhomfly libpng linbox m4ri m4rie memory_allocator mpc mpfi mpfr $(MP_LIBRARY) ntl numpy pari pip pkgconfig planarity ppl pplpy primesieve primecount primecountpy $(PYTHON) requests rw sage_conf singular symmetrica typing_extensions $(PCFILES) | $(PYTHON_TOOLCHAIN) sage_setup $(PYTHON) pythran boost_cropped

----------
All lines of this file are ignored except the first.
Expand Down
2 changes: 1 addition & 1 deletion build/pkgs/soplex/dependencies
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
$(MP_LIBRARY) mpfr boost_cropped zlib papilo | cmake
$(MP_LIBRARY) mpfr zlib papilo | cmake boost_cropped
1 change: 1 addition & 0 deletions build/pkgs/trove_classifiers/distros/conda.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
trove-classifiers
4 changes: 2 additions & 2 deletions pkgs/sage-conf_conda/setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ def run(self):
'See https://doc.sagemath.org/html/en/installation/conda.html on how to get started.')

cmd = f"cd {SAGE_ROOT} && ./configure --enable-build-as-root --with-system-python3=force --disable-notebook --disable-sagelib --disable-sage_conf --disable-doc"
cmd += ' --with-python=$CONDA_PREFIX/bin/python --prefix="$CONDA_PREFIX"'
cmd += ' $(for pkg in $(PATH="build/bin:$PATH" build/bin/sage-package list :standard: --exclude rpy2 --has-file spkg-configure.m4 --has-file distros/conda.txt); do echo --with-system-$pkg=force; done)'
cmd += ' --with-python=$CONDA_PREFIX/bin/python --prefix="$CONDA_PREFIX" --enable-system-site-packages'
Copy link
Contributor

Choose a reason for hiding this comment

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

I still think it's to early for that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just re-read the comments in the thread above from 2 months ago, I don't think there's anything that supports delaying it further.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's still "experimental" right? Given that the conda workflow is not as stable right now as we want it to be, I don't think we should add any non-stable elements to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Building the Sage distribution with --enable-system-site-packages is experimental.
Running the configure script with it is safe.

cmd += ' $(for pkg in $(PATH="build/bin:$PATH" build/bin/sage-package list :standard: --exclude rpy2 --has-file spkg-configure.m4 --has-file distros/conda.txt --exclude-dependencies); do echo --with-system-$pkg=force; done)'
print(f"Running {cmd}")
sys.stdout.flush()
if os.system(cmd) != 0:
Expand Down
Loading