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

Allow sphinx <= 7.x and other conda related improvements #35845

Merged
merged 3 commits into from
Jul 20, 2023

Conversation

tobiasdiez
Copy link
Contributor

@tobiasdiez tobiasdiez commented Jun 27, 2023

📚 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

  • The title is concise, informative, and self-explanatory.
  • 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

@tobiasdiez tobiasdiez added s: needs review s: run conda ci Run the conda workflow on this PR. labels Jun 27, 2023
conda-env: [environment, environment-optional]
# Optional environment is disabled for now as its not yet working
# environment: [environment, environment-optional]
conda-env: [environment]
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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".

Copy link
Contributor Author

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.

Copy link
Member

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.

@kiwifb
Copy link
Member

kiwifb commented Jun 28, 2023

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

@tobiasdiez
Copy link
Contributor Author

@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 develop branch. In particular, there are no issues with releases that will be installed in a year or so and would break with sphinx v8. The "sage-the-distribution as conda package" still uses an upper constraint (and is unaffected by this change): https://github.com/conda-forge/sage-feedstock/blob/155e04ade775053c64a8399f50b99265890bac92/recipe/meta.yaml#L122

@kiwifb
Copy link
Member

kiwifb commented Jun 28, 2023

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.

@mkoeppe
Copy link
Member

mkoeppe commented Jun 28, 2023

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.

@kiwifb
Copy link
Member

kiwifb commented Jun 28, 2023

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.

@tobiasdiez
Copy link
Contributor Author

The target group are developers that want to use Conda to create their dev environment. And such devs should know about conda install sphinx==x.y, or they should just wait a week with upgrading their local conda environments.

What about adding the version constraint once tests with the sphinx beta/rc show that it is breaks sage?

@isuruf
Copy link
Member

isuruf commented Jun 30, 2023

@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.

@mkoeppe mkoeppe changed the title Allow sphinx 6 and other conda related improvements Allow sphinx <= 7.x and other conda related improvements Jul 12, 2023
@mkoeppe
Copy link
Member

mkoeppe commented Jul 12, 2023

I've reduced the PR to the uncontroversial part.

@github-actions
Copy link

Documentation preview for this PR (built with commit e951e6a; changes) is ready! 🎉

@vbraun vbraun merged commit 376db2c into sagemath:develop Jul 20, 2023
11 of 12 checks passed
@mkoeppe mkoeppe added this to the sage-10.1 milestone Jul 20, 2023
@tobiasdiez tobiasdiez deleted the conda-sphinx branch July 21, 2023 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s: run conda ci Run the conda workflow on this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants