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

Add conda dev environment #33740

Closed
tobiasdiez opened this issue Apr 21, 2022 · 77 comments
Closed

Add conda dev environment #33740

tobiasdiez opened this issue Apr 21, 2022 · 77 comments

Comments

@tobiasdiez
Copy link
Contributor

We add a conda enviroment that is meant for development, i.e. includes tools that one probably needs while working on sage development such as linters, ssh (for contributing to trac) etc.

Moreover, we extract the conda-related things from docs/bootstrap to a new bootstrap-conda script in order to simplify the conda workflow (see updated documentation).

Github workflow run: https://github.com/sagemath/sagetrac-mirror/actions/runs/2268985510

CC: @isuruf @dimpase @saraedum @mkoeppe @slel

Component: build

Author: Tobias Diez

Branch: 87f360c

Reviewer: Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/33740

@tobiasdiez tobiasdiez added this to the sage-9.6 milestone Apr 21, 2022
@dimpase
Copy link
Member

dimpase commented Apr 22, 2022

comment:2

This looks quite odd to me.
Why can't the yml file in the branch be generated from what's already known about Conda packages?

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 22, 2022

comment:3

I agree, not a good idea to treat this file different from all other of our files that are generated at bootstrap time.

Also jupyter_sphinx should really be added by adding a distros/ file, not ad hoc like this.

@tobiasdiez
Copy link
Contributor Author

comment:4

Replying to @dimpase:

This looks quite odd to me.
Why can't the yml file in the branch be generated from what's already known about Conda packages?

The issue is not so much to generate the file but how it is used. Running bootstrap requires a few dependencies which however are only specified in the environment file. So its a classical chicken-egg problem.

Currently this is resolved by requiring the developers to first create a conda env with the necessary tools for bootstrap, and then later update the env with all dependencies from the env file. With my proposal the overhead occurs only when dependencies are changed (which then requires to commit the changed env file). Overall this should be less work and more convenient.

I agree, not a good idea to treat this file different from all other of our files that are generated at bootstrap time.

The dev environment is special in this respect, since its the only file that you would possibly like to use before the bootstrap.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 22, 2022

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

aa144e6Add distros/conda for jupyter_sphinx

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 22, 2022

Changed commit from 413b081 to aa144e6

@tobiasdiez
Copy link
Contributor Author

comment:6

Replying to @mkoeppe:

Also jupyter_sphinx should really be added by adding a distros/ file, not ad hoc like this.

Done.

@dimpase
Copy link
Member

dimpase commented Apr 22, 2022

comment:7

ok, but please make the initial egg (say, ./bootstrap-conda-dev) minimal - apart from m4 and other autotools, not much else is needed for ./bootstrap.

@tobiasdiez
Copy link
Contributor Author

comment:8

Mhh, having a conda environment just for the bootstrap would also work. But then one still has to run conda create sage-dev -f bootstrap-conda-dev.yml, bootstrap and conda sage-dev update -f environment.yml.

Having one conda environment config containing all necessary tools for bootstrap, compiling and running sage as well as a few additional dev tools is the most convenient, don't you think so?

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 22, 2022

comment:9

Replying to @dimpase:

ok, but please make the initial egg (say, ./bootstrap-conda-dev) minimal - apart from m4 and other autotools, not much else is needed for ./bootstrap.

+1 on just splitting the generation of the yml files from src/doc/bootstrap (it doesn't really belong there anyway) into a new script. It won't need any of the dependencies that the full bootstrap has.

@dimpase
Copy link
Member

dimpase commented Apr 23, 2022

comment:10

perhaps this should rather become a conda-forge package?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 27, 2022

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

2811559Introduce new bootstrap-conda command

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 27, 2022

Changed commit from aa144e6 to 2811559

@tobiasdiez
Copy link
Contributor Author

comment:12

Replying to @mkoeppe:

Replying to @dimpase:

ok, but please make the initial egg (say, ./bootstrap-conda-dev) minimal - apart from m4 and other autotools, not much else is needed for ./bootstrap.

+1 on just splitting the generation of the yml files from src/doc/bootstrap (it doesn't really belong there anyway) into a new script. It won't need any of the dependencies that the full bootstrap has.

Like this?

@dimpase
Copy link
Member

dimpase commented Apr 27, 2022

comment:13

that's much nicer, but where is the top-level invocation of bootstrap-conda?
Do you mean it's an alternative to ./bootstrap ?

@tobiasdiez
Copy link
Contributor Author

comment:14

I wasn't sure about this one. The procedure now looks as follows:

  • bootstrap-conda
  • conda create
  • bootstrap
  • configure
  • ...

So there is no point in invoking bootstrap-conda during bootstrap.

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 27, 2022

comment:15

Still need to invoke this so that the generated files end up in the sdist.

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 27, 2022

comment:16

(See the shell function save in bootstrap)

@dimpase
Copy link
Member

dimpase commented Apr 28, 2022

comment:17

Replying to @tobiasdiez:

I wasn't sure about this one. The procedure now looks as follows:

  • bootstrap-conda
  • conda create
  • bootstrap
  • configure
  • ...

So there is no point in invoking bootstrap-conda during bootstrap.

I thought that ./bootstrap recognises whether we're on conda, or not.
If so, it can invoke bootstrap-conda+conda create.

Otherwise, I'd like to see ./bootstrap getting an optional argument, conda,
to do the calling of bootstrap-conda+conda create and then proceeding.

@mkoeppe
Copy link
Contributor

mkoeppe commented Apr 28, 2022

comment:18

Replying to @dimpase:

I thought that ./bootstrap recognises whether we're on conda, or not.

Not a good idea. The output of bootstrap must not depend on the system configuration. It is packed into the sdist.

@dimpase
Copy link
Member

dimpase commented Apr 30, 2022

comment:19

shouldn't ./bootstrap-conda run on #!/bin/sh, just like ./bootstrap ?

@tobiasdiez
Copy link
Contributor Author

comment:21

Replying to @dimpase:

shouldn't ./bootstrap-conda run on #!/bin/sh, just like ./bootstrap ?

With sh the script is not working, so I took bash as in doc/bootstrap. If that needs to be changed, I need help as its beyond my basic shell script knowledge.

@tobiasdiez
Copy link
Contributor Author

comment:22

Replying to @dimpase:

Otherwise, I'd like to see ./bootstrap getting an optional argument, conda,
to do the calling of bootstrap-conda+conda create and then proceeding.

I like the idea to further simplify the workflow, but lets keep this ticket small and do further streamlining efforts in #33765.

@tobiasdiez
Copy link
Contributor Author

comment:23

Replying to @mkoeppe:

Still need to invoke this so that the generated files end up in the sdist.

Okay, added.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 8, 2022

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

b42f5f5Readd how to use specific python version

@tobiasdiez
Copy link
Contributor Author

comment:56

Replying to @mkoeppe:

Also, in the edits of the documentation, the information on how to select a Python version have been lost

Added again.

@mkoeppe
Copy link
Contributor

mkoeppe commented May 8, 2022

comment:57

Replying to @tobiasdiez:

Replying to @mkoeppe:

In this change:

-      $ conda create -n sage-build
+      $ conda env create --file environment.yml
       $ conda activate sage-build

I would suggest to keep the -n so that it's easy to see what needs to be changed if the user wants to use a different name

Normally the user doesn't need to change the name

It's as normal as having a second installation in a separate directory

@tobiasdiez
Copy link
Contributor Author

comment:58

Replying to @mkoeppe:

Replying to @tobiasdiez:

Replying to @mkoeppe:

In this change:

-      $ conda create -n sage-build
+      $ conda env create --file environment.yml
       $ conda activate sage-build

I would suggest to keep the -n so that it's easy to see what needs to be changed if the user wants to use a different name

Normally the user doesn't need to change the name

It's as normal as having a second installation in a separate directory

If you want to install two sages in parallel, you should also be able to find the --name arg of conda. But if you insist I can readd the name.

@mkoeppe
Copy link
Contributor

mkoeppe commented May 8, 2022

comment:59

Yes please. I added it on purpose in the original version.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 9, 2022

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

87f360cReadd name arg

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 9, 2022

Changed commit from b42f5f5 to 87f360c

@tobiasdiez
Copy link
Contributor Author

comment:61

Replying to @mkoeppe:

Yes please. I added it on purpose in the original version.

okay done

@mkoeppe
Copy link
Contributor

mkoeppe commented May 9, 2022

@mkoeppe
Copy link
Contributor

mkoeppe commented May 9, 2022

comment:63

conda's pari is not being accepted.

@tobiasdiez
Copy link
Contributor Author

comment:64

This is not specific to this branch and also occurs for develop https://github.com/sagemath/sage/runs/6236167749?check_suite_focus=true
I've opened #33828 for this.

@mkoeppe
Copy link
Contributor

mkoeppe commented May 9, 2022

Reviewer: Matthias Koeppe

@mkoeppe
Copy link
Contributor

mkoeppe commented May 9, 2022

comment:65

Thanks for checking, I agree. Then it's good to go

@tobiasdiez
Copy link
Contributor Author

comment:66

Thanks!

@mkoeppe

This comment has been minimized.

@vbraun
Copy link
Member

vbraun commented May 22, 2022

Changed branch from public/build/conda-dev-env to 87f360c

@slel
Copy link
Member

slel commented Jun 28, 2022

comment:69

The changes in this ticket made ./bootstrap -q less quiet.

Before:

$ ./bootstrap -q

After:

$ ./bootstrap -q
./bootstrap-conda:46: generate conda enviroment files

Follow-up at #34097.

@slel
Copy link
Member

slel commented Jun 28, 2022

Changed commit from 87f360c to none

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants