-
-
Notifications
You must be signed in to change notification settings - Fork 487
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
Improve conda setup #38728
Improve conda setup #38728
Conversation
Documentation preview for this PR (built with commit cf31bde; changes) is ready! 🎉 |
Is it not possible to put all the environment files into some directory, like |
Conda-lock has the possibility to create "multi-platform" lock files. That would reduce the number of lock files. I can look into this afterwards. |
Nice. Thanks. |
How am I supposed to use the default devcontainer using conda?
|
Thanks for testing. I can reproduce the issue. It fails with:
Not very informative sadly. Does it print more for you? For me it works when I call |
As you see in the screenshot, the devcontainer seems to fail somehow before even getting to building sagemath-standard... |
try to start the codespace with more space: 2-core • 8GB RAM • 32GB • 14.11 GB works for me |
2-core • 8GB RAM • 32GB is the default codespaces machine type for me. I don't know what that 14.11 GB is. Trying again. |
It failed exactly at the same step, as shown in the above screenshot. |
Try to use https://docs.github.com/en/codespaces/customizing-your-codespace/changing-the-machine-type-for-your-codespace#changing-the-machine-type with 64GB. The 14GB is the amount of space it uses for me and you can see this at https://github.com/codespaces. |
It seems that there is no machine type with 64GB disk space available for me. I am on free tier. You are using 14GB. The default machine already has 32GB. Hence the disk space should not be a problem... |
I agree, it's strange that you run out of space. In the background of the screenshot you shared, you also see that the codespace actually was created successful - just afterwards it runs out of space. But maybe we leave this for another PR? The only change I made to the codespace setup was to fix the conda initialization, which is also working for you (sage-dev is correctly initialized in your codespace, just sage failed to build). |
It seems really a space problem. There indeed appeared log messages (strangely above the "success" message) like:
Are you using a bigger machine, say with 64 GB disk space? It is really problematic if the default 32 GB machine could not cope with the conda install, since this is the default devcontainer.json... |
by the way, why is there no |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've checked on a local Linux conda install with Python 3.11, running
conda env create --file environment-3.11-linux.yml --name sage-build
mamba activate sage-build
pip install --no-build-isolation -v -v -e ./src
the resulting ./sage
seems to work.
Before building I rebased over the latest beta, 10.5.beta6
--prefix=$CONDA_PREFIX | ||
$ make | ||
|
||
|
||
.. _sec-installation-conda-develop: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about removing this section. It seems that installing sage from source within active conda environment is a supported mode. I think the files build/pkgs/*/distros/conda.txt
are there to support this mode of installing sage from source.
That this is broken now is not a sufficient reason to drop it. If we are officially to drop it, we should do it properly (including a discussion in sage-devel and removing all files build/pkgs/*/distros/conda.txt
).
So I suggest to update this section instead of removing it, for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@saraedum - as a Sage conda maintainer, any opinion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about removing this section. It seems that installing sage from source within active conda environment is a supported mode.
The supported mode is to install all dependencies with conda; what is not supported (in the sense that no one checks that it works or provides help if it doesn't) is to build the python dependencies using sage-the-distro, but install all non-python dependencies using conda. I also don't see any point in supporting such a mixed install.
I think the files
build/pkgs/*/distros/conda.txt
are there to support this mode of installing sage from source.
No, these files are only there to generate the conda environment files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... I also don't see any point in supporting such a mixed install.
I don't know... The mixed install has been in sage for years, even in broken and unsupported state. Its complete removal may need to be discussed in sage-devel if experts here have different opinions. I am not an expert.
Let's wait for @saraedum's opinion.
I think the files
build/pkgs/*/distros/conda.txt
are there to support this mode of installing sage from source.No, these files are only there to generate the conda environment files.
OK. So we should keep those files anyway.
On the other hand, those files are also used for the mixed install. No?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Building Sage in this way is the only way I've been able to successfully build Sage in a way that gives me a Jupyter kernel at the end of it. I think it's important that people are able to build Sage using conda in a way that gives them all Sage features (including a working Jupyter kernel). I would be against removing this unless we have another documented way of building Sage using conda such that you get a Jupyter kernel at the end of it.
Following the instructions "Using conda to provide all dependencies for the Sage library" doesn't give me a Jupyter kernel (or at least not one that is found by following these instructions). Building from source without conda was giving me a bunch of dependency issues and I never got it to work (I'm on Fedora), which is the entire reason I use conda to build Sage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These instructions are not needed. The sagemath jupyter kernel is automatically installed in the same conda environment once you pip installed sagemath:
$ jupyter kernelspec list Available kernels: python3 /opt/conda/envs/sage-dev-basic/share/jupyter/kernels/python3 sagemath /opt/conda/envs/sage-dev-basic/share/jupyter/kernels/sagemathIf you would want to install the kernel into a different env, we would need to add an install command accepting a prefix similar to https://ipython.readthedocs.io/en/latest/install/kernel_install.html#kernels-for-different-environments, or you use the path from
jupyter kernelspec list
to install it manually in a different venv.
Whether it's conda or not, one normally wants the kernel installed into the same (v)env as used for the build.
Such an installation is cheap, anyway, and can always be done, whenever jupyter is present.
So a general feature to make it work ought to work in conda, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually a general feature: sage always installs the kernel specs for the same venv, see https://github.com/sagemath/sage/blob/develop/src/sage_setup/command/sage_install.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These instructions are not needed. The sagemath jupyter kernel is automatically installed in the same conda environment once you pip installed sagemath:
$ jupyter kernelspec list Available kernels: python3 /opt/conda/envs/sage-dev-basic/share/jupyter/kernels/python3 sagemath /opt/conda/envs/sage-dev-basic/share/jupyter/kernels/sagemathIf you would want to install the kernel into a different env, we would need to add an install command accepting a prefix similar to https://ipython.readthedocs.io/en/latest/install/kernel_install.html#kernels-for-different-environments, or you use the path from
jupyter kernelspec list
to install it manually in a different venv.
I think manually using the path from jupyter kernelspec list
is acceptable. But in order to get the kernel like you described here I needed to remove the --editable
flag from the pip install
command. I think that should be documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could reproduce the problem with editable install. I've now added the information about the juptyer kernel in the docs.
Thanks for bringing these issues up!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could reproduce the problem with editable install. I've now added the information about the juptyer kernel in the docs.
Thanks for bringing these issues up!
The explanation looks good, thanks for addressing the feedback!
I will leave review to Dima. |
I agree. But the conda dependencies only take 4-5GB or so. Not sure how much the OS in the codespace takes, but shouldn't be more than 5GB. So the problematic part is that in order to build sage one needs >= 10-15GB. This is also about the same size where we hit issues with the build github actions a year ago...so not really a new problem.
Will be added in #36431.
Thanks for testing! |
Perhaps not new. If the conda install does not work with the default 32 GB machine, we should not set it as the default devcontainer for developers who want to use codespaces for sage development. The PR #38789 adds a devcontainer that is ready to build sage from source. We may need to consider to change the default devcontainer to it... |
I'm setting this now to positive review based on the positive review and feedback by @dimpase and @vincentmacri (thanks!), since this is urgently needed to fix the ci-issues that will be introduced by #38804. We can always readd the outdated instructions that I've removed from the docs in a later PR. |
@tobiasdiez I'm not able to get a non-editable conda install working. It builds but some basic functionality is broken. For instance:
is giving me
whereas I should be able to Can you confirm whether the non-editable conda install works for you? If it does, can you add to the documentation the steps to do a non-editable install? If it doesn't work for you, can we hold off of dropping support for the mixed install until there is a working way of building Sage with conda that gives a Jupyter kernel (or if its simple enough, include the fix in this PR?) |
This breaks
|
Strange. I've never had troubles with the non-editable install. Is the error above the only one that you experience? Does it work for you if you use |
Sorry, should be fixed now. Strange that it wasn't caught by the ci. |
Am I correct in assuming that to do a non-editable conda install you just need to change
I've gotten this error message (or perhaps a similar one) doing a few curve-related things. Not sure if it's specific to curves, but I mainly use Sage for curve stuff and so most of what I'm doing is curve-related.
What would be the steps to do that?
I'll do some more testing once this is merged in case something you did to conda here fixes it (I was trying to do the non-editable install on develop I think). If I'm still having trouble I'll open a new issue. I'd also want to figure out how to do the non-editable |
check the options of ./configure for this.
|
<!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> A few small quality of life improvements to the conda setup. Notably the environment files under `src` are now moved to the root, as the old env files there were non-functional and only confused users. Relately, removed the outdated,not working and untested instructions to use conda solely to provide the system packages for sage-the-distro. A few other improvements along the way: - Make conda in devcontainer working again by forcing mamba v1 (v2 was released a few days ago and breaks a few things related to the lock files) - Force usage of conda-forge everywhere (mixing of channels is no longer supported) ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ ] The title is concise and informative. - [ ] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#38728 Reported by: Tobias Diez Reviewer(s): Dima Pasechnik, Kwankyu Lee, Tobias Diez, Vincent Macri
<!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> A few small quality of life improvements to the conda setup. Notably the environment files under `src` are now moved to the root, as the old env files there were non-functional and only confused users. Relately, removed the outdated,not working and untested instructions to use conda solely to provide the system packages for sage-the-distro. A few other improvements along the way: - Make conda in devcontainer working again by forcing mamba v1 (v2 was released a few days ago and breaks a few things related to the lock files) - Force usage of conda-forge everywhere (mixing of channels is no longer supported) ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ ] The title is concise and informative. - [ ] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#38728 Reported by: Tobias Diez Reviewer(s): Dima Pasechnik, Kwankyu Lee, Tobias Diez, Vincent Macri
Exactly! You might also try the new meson-based built system (which will be the default hopefully very soon) by using |
<!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> A few small quality of life improvements to the conda setup. Notably the environment files under `src` are now moved to the root, as the old env files there were non-functional and only confused users. Relately, removed the outdated,not working and untested instructions to use conda solely to provide the system packages for sage-the-distro. A few other improvements along the way: - Make conda in devcontainer working again by forcing mamba v1 (v2 was released a few days ago and breaks a few things related to the lock files) - Force usage of conda-forge everywhere (mixing of channels is no longer supported) ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ ] The title is concise and informative. - [ ] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#38728 Reported by: Tobias Diez Reviewer(s): Dima Pasechnik, Kwankyu Lee, Tobias Diez, Vincent Macri
<!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> A few small quality of life improvements to the conda setup. Notably the environment files under `src` are now moved to the root, as the old env files there were non-functional and only confused users. Relately, removed the outdated,not working and untested instructions to use conda solely to provide the system packages for sage-the-distro. A few other improvements along the way: - Make conda in devcontainer working again by forcing mamba v1 (v2 was released a few days ago and breaks a few things related to the lock files) - Force usage of conda-forge everywhere (mixing of channels is no longer supported) ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ ] The title is concise and informative. - [ ] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#38728 Reported by: Tobias Diez Reviewer(s): Dima Pasechnik, Kwankyu Lee, Tobias Diez, Vincent Macri
<!-- ^ Please provide a concise and informative title. --> <!-- ^ Don't put issue numbers in the title, do this in the PR description below. --> <!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method to calculate 1 + 2". --> <!-- v Describe your changes below in detail. --> <!-- v Why is this change required? What problem does it solve? --> <!-- v If this PR resolves an open issue, please link to it here. For example, "Fixes sagemath#12345". --> A few small quality of life improvements to the conda setup. Notably the environment files under `src` are now moved to the root, as the old env files there were non-functional and only confused users. Relately, removed the outdated,not working and untested instructions to use conda solely to provide the system packages for sage-the-distro. A few other improvements along the way: - Make conda in devcontainer working again by forcing mamba v1 (v2 was released a few days ago and breaks a few things related to the lock files) - Force usage of conda-forge everywhere (mixing of channels is no longer supported) ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [ ] The title is concise and informative. - [ ] The description explains in detail what this PR is about. - [ ] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation and checked the documentation preview. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#38728 Reported by: Tobias Diez Reviewer(s): Dima Pasechnik, Kwankyu Lee, Tobias Diez, Vincent Macri
echo " - $pkg" | ||
done | ||
) > environment-optional-template.yml | ||
) > environment-template.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tobiasdiez This breaks the build.
On the left hand side you read from environment-template.yml
, but on the right hand side you >
into that same file. On my machine the file is erased before it's read.
Fortunately the fix is simple: since both are actually the same file, you can just combine the two blocks into one.
As a side note, you didn't fix https://doc-pr-38728--sagemath.netlify.app/html/en/installation/meson to say |
A few small quality of life improvements to the conda setup.
Notably the environment files under
src
are now moved to the root, as the old env files there were non-functional and only confused users. Relately, removed the outdated,not working and untested instructions to use conda solely to provide the system packages for sage-the-distro.A few other improvements along the way:
📝 Checklist
⌛ Dependencies