Opened 11 months ago

Closed 9 months ago

#32904 closed defect (fixed)

Add version constraints for sage-conf, clean the sage-conf sdist

Reported by: Tobias Diez Owned by:
Priority: major Milestone: sage-9.5
Component: build Keywords:
Cc: Matthias Köppe, John Palmieri, Dima Pasechnik, gh-kliem Merged in:
Authors: Matthias Koeppe Reviewers: Tobias Diez, Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: 25cd86b (Commits, GitHub, GitLab) Commit: 25cd86b788d68ea5b84d927735975f48d6fea906
Dependencies: #29039 Stopgaps:

Status badges

Description (last modified by Matthias Köppe)

Currently, if you try to install sagemath-standard using pip install ./src (from SAGE_ROOT) then this fails with the following error

  Installing build dependencies: started
  Installing build dependencies: finished with status 'done'
  Getting requirements to build wheel: started
  Getting requirements to build wheel: finished with status 'done'
    Preparing wheel metadata: started
    Preparing wheel metadata: finished with status 'error'

  Getting requirements to build wheel ... done
    Running command /home/tobias/sage/src/.venv/bin/python /home/tobias/sage/src/.venv/lib/python3.8/site-packages/pip/_vendor/pep517/in_process/_in_process.py prepare_metadata_for_build_wheel /tmp/tmpj9q6knbr
    Exception while cythonizing source files: FileNotFoundError(2, 'No such file or directory')
    Building interpreters for fast_callable
    ************************************************************************
    Traceback (most recent call last):
      File "/home/tobias/sage/src/.venv/lib/python3.8/site-packages/pip/_vendor/pep517/in_process/_in_process.py", line 349, in <module>
        main()
      File "/home/tobias/sage/src/.venv/lib/python3.8/site-packages/pip/_vendor/pep517/in_process/_in_process.py", line 331, in main
        json_out['return_val'] = hook(**hook_input['kwargs'])
      File "/home/tobias/sage/src/.venv/lib/python3.8/site-packages/pip/_vendor/pep517/in_process/_in_process.py", line 151, in prepare_metadata_for_build_wheel
        return hook(metadata_directory, config_settings)
      File "/tmp/pip-build-env-mke9oai7/overlay/lib/python3.8/site-packages/setuptools/build_meta.py", line 174, in prepare_metadata_for_build_wheel
        self.run_setup()
      File "/tmp/pip-build-env-mke9oai7/overlay/lib/python3.8/site-packages/setuptools/build_meta.py", line 158, in run_setup
        exec(compile(code, __file__, 'exec'), locals())
      File "setup.py", line 114, in <module>
        aliases=cython_aliases(),
      File "/tmp/pip-req-build-tz70fj41/sage/env.py", line 557, in cython_aliases
        ecl_cflags = subprocess.run([ECL_CONFIG, "--cflags"], check=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True).stdout.split()
      File "/usr/lib/python3.8/subprocess.py", line 493, in run
        with Popen(*popenargs, **kwargs) as process:
      File "/usr/lib/python3.8/subprocess.py", line 858, in __init__
        self._execute_child(args, executable, preexec_fn, close_fds,
      File "/usr/lib/python3.8/subprocess.py", line 1704, in _execute_child
        raise child_exception_type(errno_num, err_msg, err_filename)
    FileNotFoundError: [Errno 2] No such file or directory: '/Users/mkoeppe/s/sage/sage-rebasing/worktree-dist/.tox/local-macos-10.15-nohomebrew-python3_xcode/local/bin/ecl-config'

In this ticket, we add version constraints for the build-time and run-time dependencies on sage_conf, and clean up the sdist of sage_conf, in particular so that it no longer ship configuration artifacts from the machine where the sdist was built.

Change History (36)

comment:1 Changed 11 months ago by Matthias Köppe

Summary: Installation of sagemath-standard with build isolutionInstallation of sagemath-standard with build isolation

comment:2 Changed 11 months ago by Matthias Köppe

Which wheel of sage_conf are you referring to? On PyPI there are only sdists, not wheels.

comment:3 Changed 11 months ago by Matthias Köppe

The log in the ticket description is not very useful. Best to use pip install -v -v so that sufficient detail is presented.

comment:4 Changed 11 months ago by Matthias Köppe

Status: newneeds_info

comment:5 Changed 11 months ago by Tobias Diez

I looked a bit deeper into this, and the problem is that https://pypi.org/project/sage-conf/9.4.post3/#files indeed contains your config (and is automatically used as there is no version specification in the pyproject file). But as far as I could see this is improved in the most recent pre-release.

But the main question remains, I believe: How to specify the correct build config for sage if you use build isolation (which is the preferred way of installing packages).

comment:6 Changed 11 months ago by Matthias Köppe

What's missing is certainly a version constraint for the sage_conf dependency (both in pyproject.toml and setup.cfg). We should add this via build/pkgs/sage_conf/install-requires.txt.

comment:7 Changed 11 months ago by Matthias Köppe

Once there is a proper version constraint, if a correct version of sage_conf is already installed, then the isolated build environment should take sage_conf from the existing installation instead of building a fresh one from the PyPI sdist.

comment:8 Changed 11 months ago by Matthias Köppe

Dependencies: #29039

Looking at the 9.4.post3 tarball, yes, this sdist is not clean. These files (all artifacts generated by configure) should not be included:

  • sage_root/src/bin/sage-env-config
  • sage_root/src/bin/sage-src-env-config
  • sage_root/build/bin/sage-build-env-config
  • sage_root/build/make/Makefile-auto
  • sage_root/pkgs/sage-conf/sage_conf.py
  • sage_root/pkgs/sage-conf/bin/sage-env-config

Also sage_root/src/bin has a bunch of other scripts that don't belong there.

Some of this would just go away by running the sdist building in a clean environment (#32062 does this on GH Actions).

But it's best to fix it using MANIFEST.in.

Let's use the present ticket to address this.

comment:9 Changed 11 months ago by Matthias Köppe

Branch: u/mkoeppe/installation_of_sagemath_standard_with_build_isolation

comment:10 in reply to:  description Changed 11 months ago by Matthias Köppe

Commit: b1301b172c7095ab94b95735ed7b240220213cc8

Replying to gh-tobiasdiez:

One option, recommended by https://setuptools.pypa.io/en/latest/deprecated/distutils/configfile.html?highlight=header#writing-the-setup-configuration-file, is

As long as that information is fairly simple—a list of directories to search for C header files or libraries, for example—then providing a configuration file, setup.cfg, for users to edit is a cheap and easy way to solicit it.

Yeah, no, these are ancient practices and we will not start with this


Last 10 new commits:

02505capkgs/sage-conf_pypi/MANIFEST.in: Update from #31396
aeca1a0Makefile (pypi-sdists): Add sage_setup
cd3dce9pkgs/sage-conf_pypi/setup.py: Update directory of configured sage_conf.py
c695921pkgs/sage-conf/sage_conf.py.in (SAGE_SPKG_WHEELS): Update so it works if SAGE_VENV != SAGE_LOCAL
cebcda3Merge tag '9.5.beta4' into t/29039/pip_installable_package_sage_bootstrap
0026892Merge tag '9.5.beta5' into t/29039/pip_installable_package_sage_bootstrap
2a4a323Merge tag '9.5.beta7' into t/29039/pip_installable_package_sage_bootstrap
e24090aMerge #29039
5aba4a1build/pkgs/sage_conf/install-requires.txt: Add version constraint
b1301b1src/{pyproject.toml.m4, setup.cfg.m4}: Include version constraint for sage-conf

comment:11 in reply to:  description Changed 11 months ago by Matthias Köppe

Replying to gh-tobiasdiez:

one cannot control only the version of the other build_requires packages to installed, but not where they are coming from.

Actually one can, at least in some use cases, by using --no-index --find-links .... See for example pkgs/sagemath-standard/tox.ini.

This makes it hard to use sage_conf as a build_requires dependency, since it will be usually fetched from pypi and thus doesn't take the users config into account.

Actually the version of sage-conf on PyPI comes from pkgs/sage-conf_pypi (from #29039) and runs the configure script upon installation.

comment:12 Changed 11 months ago by Matthias Köppe

Authors: Matthias Koeppe
Status: needs_infoneeds_work
Summary: Installation of sagemath-standard with build isolationAdd version constraints for sage-conf, clean the sage-conf sdist

comment:13 in reply to:  7 Changed 11 months ago by Tobias Diez

Replying to mkoeppe:

Once there is a proper version constraint, if a correct version of sage_conf is already installed, then the isolated build environment should take sage_conf from the existing installation instead of building a fresh one from the PyPI sdist.

I thought so too but it turns out that pip specifies --ignore-installed when constructing the isolated build environment, see https://github.com/pypa/pip/issues/6482. Thus it will most likely fetch pypi, or if you are lucky use the a locally cached wheel. So that doesn't seem to work.

The version constraint makes sense in either case in my opinion.

comment:14 Changed 11 months ago by git

Commit: b1301b172c7095ab94b95735ed7b240220213cc81dbe02358eec3b1f1f45e8e641d139f4d80ba3c7

Branch pushed to git repo; I updated commit sha1. New commits:

1dbe023pkgs/sage-conf_pypi/sage_root: Reduce src/bin to the needed files

comment:15 Changed 11 months ago by git

Commit: 1dbe02358eec3b1f1f45e8e641d139f4d80ba3c725cd86b788d68ea5b84d927735975f48d6fea906

Branch pushed to git repo; I updated commit sha1. New commits:

25cd86bpkgs/sage-conf_pypi/MANIFEST.in: Exclude unneeded files

comment:16 Changed 11 months ago by Matthias Köppe

Status: needs_workneeds_review

comment:17 Changed 10 months ago by Matthias Köppe

Cc: Dima Pasechnik gh-kliem added

comment:18 Changed 10 months ago by Dima Pasechnik

After this patch, it ends up with

Successfully installed sagemath-standard-9.5b7

What's the correct name of the module it installs? (Neither - not . are allowed in names, as far as I understand, and indeed importing the name above fails).

comment:19 Changed 10 months ago by Matthias Köppe

Read the documentation added in https://trac.sagemath.org/ticket/32899, it explains all of that

comment:20 Changed 10 months ago by Tobias Diez

Does this really work for you with build isolation? For me it still fetches the locally-cached version or the one from pypi if the version of sage_conf specified in src/pyproject.toml is available there. That's also the expected behavior as I've outlined above in https://trac.sagemath.org/ticket/32904#comment:7

Thus adding the version constraint might work for production (i.e. external people downloading both sagemath and sage-conf from pypi) but not for development. Moreover, if I understand the documentation correctly than sage-conf from pypi is very expensive to install and needs to be done twice (once to be in the final venv and once to build sagemath-standard). Thus this is also not ideal.

comment:21 Changed 10 months ago by Matthias Köppe

Yes, installing the version from PyPI is expensive, as it is equivalent to building the Sage distribution from source. It is only done once, though, as it reuses

If you use --no-index, it will not fetch the version from PyPI.

If you combine it with --find-links=SAGE_VENV/var/lib/sage/wheels, then it will take the prebuilt sage_conf wheel from there, both for build and for install.

(Hence, #32913 - Revert #32713 (Apply "configure --enable-editable" to other packages) for sage-conf, sage-setup... waiting for review)

comment:22 in reply to:  19 Changed 10 months ago by Dima Pasechnik

Replying to mkoeppe:

Read the documentation added in https://trac.sagemath.org/ticket/32899, it explains all of that

I don't think it really answers my question - what is the name of the Python module being installed here?

comment:23 Changed 10 months ago by Matthias Köppe

It does:

  • "A distribution that provides Python modules in the :mod:sage.* namespace, say mainly from :mod:sage.PAC.KAGE, should be named sagemath-DISTRI-BUTION."
  • "Other distributions should not use the prefix sagemath- in the distribution name."

Therefore, sagemath-standard provides modules in the sage.* namespace. Namely, all of the Sage library

comment:24 in reply to:  21 ; Changed 10 months ago by Tobias Diez

Replying to mkoeppe:

Yes, installing the version from PyPI is expensive, as it is equivalent to building the Sage distribution from source. It is only done once, though, as it reuses

As I said above, pip specifies ignore-installed while constructing the build environment and thus will build sage-conf twice. Please convince yourself if you don't believe me: https://github.com/pypa/pip/blob/main/src/pip/_internal/build_env.py#L221

This is discussed in many issues on the pip repo, some examples: https://github.com/pypa/pip/issues/6482 https://github.com/pypa/pip/issues/9542 https://github.com/pypa/pip/issues/9439 All of these issues deal with (different) problems that come from taking "build isolation" as literally as pip is doing. The two major issues being that build and runtime dependencies are downloaded and build twice, and that there is no guarantee that build and runtime dependencies use the same version. It also seems that this is not something that will be fixed relatively soon.

In particular, the following will definitely build sage-conf twice, especially with the changes in this ticket:

pip install sage-conf==9.5
pip install sagemath==9.6

That may look like a silly example and no user would probably run this directly, but such a situation may happen quickly if you use packages that specify different but compatible versions of sagemath; or if you e.g. build sage-conf yourself locally; or if you upgrade sagemath later (which would trigger another installation of sage-conf). Also note that sagemath 9.6 will probably work well with sage-conf 9.5 since the latter is after installation nothing more than a glorified configuration file and thus rarely changes (once the changes to the build system due to the modularization settle down).

Your suggestion to use no-index or find-links works for development, but not for end-user that want to use the sagemath package from pypi.

The options I see are:

  • Accept that sage doesn't work with build isolation (which would be unfortunate)
  • Split sage-conf into build and runtime configuration. There is still an overlap (and thus duplication) but at built time sage needs less dependencies than at runtime. In addition, maybe directly incorporate the build-time sage-conf in the sagemath package.
  • Merge sage-conf in sagemath (I guess there was a reason to split it out in the first place; which?)
  • Explore different options to pass the built-time configuration to sagemath, e.g. using a script that sets the correct env variables or writes an env file; use package-config as described in PEP 517 to set the configuration or point to a config file.

comment:25 in reply to:  24 ; Changed 10 months ago by Matthias Köppe

Replying to gh-tobiasdiez:

Replying to mkoeppe:

Yes, installing the version from PyPI is expensive, as it is equivalent to building the Sage distribution from source. It is only done once, though, as it reuses

As I said above, pip specifies ignore-installed while constructing the build environment and thus will build sage-conf twice.

It reuses SAGE_LOCAL, which resides in a subdirectory of ~/.sage, so it does NOT build the distribution twice.

comment:26 Changed 10 months ago by Matthias Köppe

With the present ticket, sagemath-standard declares the same version constraints on sage_conf for both build and install.

How strict this version constraint has to be will have to be seen as we gain more experience with it. Fixing it to the same version as sagemath-standard will be safe but perhaps unnecessarily strict.

comment:27 in reply to:  24 Changed 10 months ago by Matthias Köppe

Replying to gh-tobiasdiez:

  • Merge sage-conf in sagemath

That would be equivalent to just pinning the version of sage_conf with ==, and is less flexible.

Last edited 10 months ago by Matthias Köppe (previous) (diff)

comment:28 Changed 10 months ago by Matthias Köppe

Description: modified (diff)

comment:29 Changed 10 months ago by Matthias Köppe

I have narrowed the ticket description to what is being done on the branch.

For more general discussion of how to improve configuration, let's use Meta-ticket #21707 ("Split sage-env into 5 to clean up sage configuration")

comment:30 in reply to:  25 ; Changed 10 months ago by Tobias Diez

Replying to mkoeppe:

Replying to gh-tobiasdiez:

Replying to mkoeppe:

Yes, installing the version from PyPI is expensive, as it is equivalent to building the Sage distribution from source. It is only done once, though, as it reuses

As I said above, pip specifies ignore-installed while constructing the build environment and thus will build sage-conf twice.

It reuses SAGE_LOCAL, which resides in a subdirectory of ~/.sage, so it does NOT build the distribution twice.

So you mean that sage-conf would be installed twice, but the second installation is considerably quicker since most of the built output is 'cached' in SAGE_LOCAL?

That's nice and probably takes care of the issue for end-user. For development, the workarounds of not using build isolation or no-index/find-files are working.

The ticket generally looks good to me, especially with the narrowed focus. Two questions:

sage-conf ~= 9.5.b6

shouldn't this be == to reduce the potential for issues in the future? Once a bit of experience is collected one may play around and see if less strict constraints are sufficient?

What's the purpose of the added symlinks?

comment:31 in reply to:  30 Changed 10 months ago by Matthias Köppe

Replying to gh-tobiasdiez:

Replying to mkoeppe:

It reuses SAGE_LOCAL, which resides in a subdirectory of ~/.sage, so it does NOT build the distribution twice.

So you mean that sage-conf would be installed twice, but the second installation is considerably quicker since most of the built output is 'cached' in SAGE_LOCAL?

Yes, that's right.

comment:32 in reply to:  30 Changed 10 months ago by Matthias Köppe

Replying to gh-tobiasdiez:

What's the purpose of the added symlinks?

The branch is replacing the symlink to src/bin by separate symlinks to the scripts that it actually needs

comment:33 Changed 10 months ago by Tobias Diez

Reviewers: Tobias Diez

okay, thanks for the clarification.

LGTM. Dima, feel free to set it to positive review if it's working for you.

comment:34 Changed 10 months ago by Dima Pasechnik

Reviewers: Tobias DiezTobias Diez, Dima Pasechnik
Status: needs_reviewpositive_review

OK

comment:35 Changed 10 months ago by Matthias Köppe

Thanks!

comment:36 Changed 9 months ago by Volker Braun

Branch: u/mkoeppe/installation_of_sagemath_standard_with_build_isolation25cd86b788d68ea5b84d927735975f48d6fea906
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.