-
-
Notifications
You must be signed in to change notification settings - Fork 503
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
Allow sphinx <= 7.x and other conda related improvements #35845
Conversation
conda-env: [environment, environment-optional] | ||
# Optional environment is disabled for now as its not yet working | ||
# environment: [environment, environment-optional] | ||
conda-env: [environment] |
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.
Let's maybe merge #35593?
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 think you can simply revert this change in your PR once it's ready.
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.
sure.
@@ -1 +1 @@ | |||
sphinx<6,>=5.2 |
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.
We need to keep an upper bound.
sphinx<8
would be appropriate as the fixes for 7 have been done.
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.
why? I would only add a version constraint if there are really known issues.
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.
Because we know that in every Sphinx upgrade we had to fix something critical.
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.
That probably applies to many dependencies, but almost none of them have upper constraints in the conda info.
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.
No, I am saying that it specifically applies to Sphinx.
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.
all of this applies to cython as well in an even more extreme way, and there we don't specify any upper bound.
Please check the facts before making such claims; it really makes no sense to discuss at this level. We do set an upper bound for Cython in install-requires.txt. (There we know already that the next major will break things. That's #29863.
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.
finally, its not conda specific.
We agree here -- I of course also object to removing the upper bound from build/pkgs/sphinx/install-requires.txt
.
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.
conda is marked experimental, so a few rough edges are acceptable.
That's a terrible argument; certainly we don't want to keep it "experimental".
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.
Thanks for your input, I see your point but don't agree with it.
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.
Which point? I've made many.
Very long bickering to read through early in the morning. Part of me feels like to leave you with your mess on conda since you clearly claim it is your own problem. And I guess it is fine so long that you expect conda users to install the whole of sage via conda. Then I agree, this is your problem. The issue is for the mid-way point so to speak, those files are to help you install sage manually with specific instructions and tips for your distribution of choice. And once it is released, it is forever for that release. The future of the interfaces of many sage component is truly unpredictable. Upper bound setting is a dark art requiring careful choosing. If you release often, you have a lot of opportunity to shift them in new releases. If you release once in a blue moon, you will get complaints. I think that sage release often enough that we can move some constraints relatively fast. That's never fast enough for some people. For those people, I recommend they use Volker's merging branch :P |
@kiwifb Please note that these conda files are only used to generate a development environment. So expect everyone that uses these files to be developers and being on the tip of the |
If this is your only user base for those files, I consider that a bit sloppy, but yes - your problem. Do we use these for automated testing? Nothing worse than a test case being stuck and waiting for a fix to be merged. It pollutes your CI. |
It's not Tobias's individual problem though. This way of setting up a Sage development environment, still marked as experimental, is actually my go-to installation method for users who work with me (unless they need to use optional packages that are not available in conda). And as I said above already, "we cannot assume that the people who follow our instructions for installing Sage are already familiar with these conda-specific details." This very much applies to the user base that I'm working with. The way to get rid of the "experimental" label is exactly to apply the principle of "eating your own dog food (by proxy)" and then fixing problems as they arise. See for example the recent PRs that add a documented procedure to build the documentation in this mode. |
Both of those last comments are very important. Before making decision we need to know our user base for the file. And we can see that Tobias and yours are not the same. Tobias does not know about your users. |
The target group are developers that want to use Conda to create their dev environment. And such devs should know about What about adding the version constraint once tests with the sphinx beta/rc show that it is breaks sage? |
@tobiasdiez, at the same time, such devs could update their sphinx packages as well. Like @kiwifb, the issue for me with upper bounds is the release schedule. We can try to keep an upper bound for sphinx for a few releases of sphinx and see whether we fall behind a lot in updating that upper bound. |
I've reduced the PR to the uncontroversial part. |
Documentation preview for this PR (built with commit e951e6a; changes) is ready! 🎉 |
📚 Description
Allow sphinx 6 and 7 to be installed (using conda) since after #35658 this no longer leeds to errors.
Also disable the
enviroment-optional
matrix testing since this is currently broken (can easily be renenabled in the PR that fixes it), and always run the test step in the conda workflow.📝 Checklist
⌛ Dependencies