-
-
Notifications
You must be signed in to change notification settings - Fork 480
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
Update Conda installation instructions to use Miniforge #37588
Conversation
This looks good to me and nicely aligns with upstream recommendations. Also, as mentioned on sage-devel, I personally found that the new recommended "miniforge" approach works very well for installing the correct version of sage. I also did further testing installing tensorflow and pytorch into the same environment, and all worked well together. |
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 wrote my positive review in a comment on the PR.
Documentation preview for this PR (built with commit 40de356; changes) is ready! 🎉 |
Thanks for checking this all works on Linux, @williamstein Do you (or someone at SMI) have access to a Windows machine they can test the equivalent of #37184 for Windows on? Then either a change based on that ticket could be a second commit here, or another PR. |
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.
Looks good.
@kcrisman Sorry for the confusion caused over at #37534 and #37184. For the background: Matthias blocked me and other members of the sage community on github. This means that I cannot post on his PRs (changing the label is the only way I can interact with them). I've asked the sage-abuse team to please forward my reviewer comments. As when playing Chinese whispers (or telephone in the US?), the main message got lost and reduced to a "-1". So let's try again: For #37184: Replacing mambaforge was one of the points (which now fortunately has been fixed). The main criticism I have, however, is that large parts of the documentation are now copy & paste. This makes it harder to maintain them in the future as they are prone to diverge over time. My suggestion is to instead use https://docutils.sourceforge.io/docs/ref/rst/directives.html#replace to reuse text without duplication (or perhaps extract it to a new file and include it twice). For #37534: -1 on recommending devcontainers as a means for using sage in such a prominent place. I actually tried this setup for some time but it's quite cumbersome. I can see it working for someone that uses vscode for all sage work and who has all documents in one directory. But it's not suitable for most people based on my experience. The installation instructions should give a clear path that is suitable for most users, and not be a labyrinth of 50 different installation options. I suggest to put this addition in a completely new section/document (perhaps "Using SageMath via Developer Containers") and then link to that section in the "Installation instructions" as a possible alternative. The whole devcontainer setup is also an option for linux and macos users (with the same limitations). |
Thanks everyone for the review! |
Thanks, and in the meantime I've been brought a bit up to speed.
To be sure. Obviously fixing that would be part of a much larger effort, presumably there are multiple such places. (I presume we haven't used that directive before in our doc, yet, which may be an old assumption.)
That's my gut feeling as well, but I have to test it out first. |
Of course, I only mean that there should be no new duplication introduced in the PR (not that all existing duplications should be fixed). The WSL setup instructions are added twice, and the instructions on how to setup the conda environment is essentially also a copy & paste of the instructions in the conda section. |
Of course, I only mean that there should be no new duplication introduced
in the PR (not that all existing duplications should be fixed). The WSL
setup instructions are added twice, and the instructions on how to setup
the conda environment is essentially also a copy & paste of the
instructions in the conda section.
I see. I guess I understand the sentiment, but don't feel that this
documentation upgrade is the place to introduce a change of this type. We
(meaning Sage) have often teetered pretty close to the edge of letting the
perfect be the enemy of the good (or acceptable, or something).
A good compromise would be to immediately start a new PR that introduces
this new (to Sage, I guess) syntax, with this and perhaps one or two other
well-chosen exemplars as test cases. Then a lot of people can try out
building doc with that. I would seriously hesitate to improve the "forge"
doc in this way on its own, though. The bleeding edge can bleed you out,
to make a dubious metaphor.
And of course another good option would be to start by changing the
developer guide to have some proscription on introducing new duplications,
with lots of advertising on Sage-devel (though how would one enforce it...
) Anyway, a discussion for another time, and probably for others than me,
since I haven't written much doc lately (and that which I did surely has
many duplications!).
|
We already use
This sounds like a very good idea! |
…orge <!-- ^ 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". --> Based on the discussion in https://groups.google.com/g/sage- devel/c/GaQHdu0Q6UU and sagemath#37184, we now follow the offical recommendation and use Miniforge over the soft- deprecated Mambaforge. Since we are exclusively testing on miniforge, this change also aligns the docs with the ci. Moreover, I've removed the recommendation to install mamba (using `conda install mamba`) which is not recommended according to the [offical docs](https://mamba.readthedocs.io/en/latest/installation/mamba- installation.html#existing-conda-install-not-recommended). ### 📝 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 accordingly. ### ⌛ 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#37588 Reported by: Tobias Diez Reviewer(s): Matthias Köppe, roed314, William Stein
ab1a517
<!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> We streamline the installation instructions for Windows by including conda-forge instructions. [Sage is stuck at version 9.5 in Ubuntu](https://repology.org/project/sagemath/versions), the default Linux distribution on WSL, so this should not be our primary recommendation. Preview: https://deploy-preview-37184-- sagemath.netlify.app/html/en/installation/ <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> ## Alternatives / follow-ups: - Include a link to Microsoft Store search for Linux distros - https://www.microsoft.com/en- us/search/shop/apps?price=0&q=linux&devicetype=pc - Include a link to Arch WSL in Microsoft Store - https://apps.microsoft.com/detail/9MZNMNKSM73X?hl=en-us&gl=US -- this is the only Linux distro in the Microsoft Store that has up-to-date SageMath packaging - Include a link to "Import any Linux distribution to use with WSL" -- https://learn.microsoft.com/en-us/windows/wsl/use-custom-distro -- these instructions should work with any Docker image, so many options - Create a WSL launcher app -- https://github.com/Microsoft/WSL- DistroLauncher, and document how users can "sideload" it -- https://learn.microsoft.com/en-us/windows/wsl/build-custom- distro#sideloading-a-custom-linux-distro-package - Create a WSL launcher app and publish it to the Microsoft Store -- https://github.com/Microsoft/WSL-DistroLauncher?tab=readme-ov- file#publishing - sagemath#34286 - sagemath#37534 ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [x] 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 accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> - Depends on sagemath#37588 (merged here) <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#37184 Reported by: Matthias Köppe Reviewer(s): kcrisman, Matthias Köppe, Tobias Diez
Based on the discussion in https://groups.google.com/g/sage-devel/c/GaQHdu0Q6UU and #37184, we now follow the offical recommendation and use Miniforge over the soft-deprecated Mambaforge. Since we are exclusively testing on miniforge, this change also aligns the docs with the ci.
Moreover, I've removed the recommendation to install mamba (using
conda install mamba
) which is not recommended according to the offical docs.📝 Checklist
⌛ Dependencies