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

myst-nb 0.13.0, pip check, update dependencies, add maintainers #4

Closed
wants to merge 5 commits into from

Conversation

bollwyvl
Copy link
Contributor

@bollwyvl bollwyvl commented Feb 7, 2021

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

In trying to set up a development environment for jupyter-book, I've run into issues with the dependency that uses the tilde operator. this uses the more explicit >=x,<x+1 form.

Also adds pip check, given the number of deps we're dealing with here.

Since I'm not super in need of this package until jupyter-book is available, i haven't added myself as a maintainer, but would be happy to do so at that time!

This also adds all the maintainers of jupyter-book (and a growing number of its dependencies), which should help keep things releasing in a timely manner.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Feb 7, 2021

@conda-forge-admin please rerender

@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2021

Hi! This is the friendly automated conda-forge-webservice.
I tried to rerender for you, but it looks like there was nothing to do.

recipe/meta.yaml Outdated
@@ -25,7 +25,7 @@ requirements:
- ipywidgets >=7.0.0,<8
- jupyter-cache >=0.4.1,<0.5
- jupyter-sphinx ==0.3.1
- myst-parser ~=0.13.3
- myst-parser >=0.13.3,<0.14.0

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aren't these two things equivalent? why change it? we use the ~= syntax throughout all of the other executablebooks repositories so this would be a break from convention

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

despite whatever EB does, i see a fair number of these in the wild that are just plain wrong. meanwhile, across the more than 10k conda-forge repositories, where we ship stuff from all over that works together, the > syntax is widely used because it's:

  • understood by humans from a wider swathe of educational backgrounds (e.g. second or third year maths)
    • ~ and ^ have wildly different meanings in other settings
  • specifies an actual, numeric upper and lower bounds helps people when they get unsolveable conda errors or pip check failures, rather than presenting them with a puzzle

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

despite whatever EB does

I appreciate your help with these distributions. But it is an EBP practice to use ~, and a perfectly legitimate part of the requirement specification.
The requirements here should directly replicate those found in the main repository (setup.cfg), and I'm going to have to put my foot down here, and say that is the only practice that I am accepting

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a reasonable case to be made in either direction on the specific syntax. Since these conda-forge recipes are likely going to have participation from folks in the EBP world, I think there's a strong value in just having consistency for how things are done. If the syntax should change, I think that decision should be made at the community-level rather than deciding in ad-hoc fashion in repositories here and there

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is more that putting upper bounds at all causes problems across conda packages because it makes it harder to solve for dependencies in any given environment. EBP may have very strict protocols about following semantic versioning but most projects don't, so conda forge has to work with the lowest common denominator in general, i.e. don't assume version numbers are semantic versioned, only that they are in sequence. The general problem with upper bounds is that you can't predict the future and know a priori if that version really is an upper bound. The best method is to go back and add upper bounds to old conda packages after a new version is released and the upper bound is definitively known. The ~ syntax seems to be mostly tied to the idea of semantic versioning and thus isn't such a nice fit for conda dependency trees that span all packages in an environment.

This article does a good job at explaining the issues with semantic versioning: https://hynek.me/articles/semver-will-not-save-you/ if you are curious.

The best thing to do is to leave the upper bounds off on new releases. But, if you really want to predict upper bounds on new releases, then making them explicit is better than not, as per @bollyyvl's suggestions. And this is because conda packages have to be as widely compatible with other packages in an environment as possible, not just EBP's ecosystem of packages.

Many, many conda recipes deviate the build specifications from the build specs in the project's own testing environment to maximize compatibility. Same goes with debian, fedora, homebrew, etc.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This article does a good job at explaining the issues with semantic versioning:

yes I have read this and also all the discussion around it: https://news.ycombinator.com/item?id=26314620

Copy link
Contributor

@chrisjsewell chrisjsewell Sep 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is more that putting upper bounds at all causes problems across conda packages because it makes it harder to solve for dependencies in any given environment.

yep thats great; it should be harder to solve; we are trying to specify an environment where the package will actually work; it is almost definite that it will not work with new major versions of packages, given that they will be breaking in nature.
What is the point of being able to solve for an environment that does not actually work?

I would strongly suggest that you are trying to "fix" an issue that has never existed; I can't think of any bugs that have been opened in EBP about a faulty/non-solvable conda distribution,
but I could point out many, many times these would have broken if we had allowed for a newer version of a package to be installed:

  • Every package in the jupyter-book stack would have broken when I released markdown-it-py v1
  • myst-nb would break on every major release of myst-parser I have released
  • Jupyter-book would break when we merge this PR

...

@choldgraf
Copy link

@bollwyvl I'm not really sure whether this is ready to be merged or not...I don't think that we have the reviewer bandwidth to keep up with these conda-forge versions as well. Also I don't have write access to this repository so I'm not sure I can do anything myself

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Sep 3, 2021

Also I don't have write access to this repository so I'm not sure I can do anything myself

I'll take a look at re-syncing the deps on #10, and will add the @conda-forge/jupyter-book team...

bandwidth to keep up with these conda-forge versions

Right, that's the goal of having teams (rather than individuals) to spread the tasks more broadly, and trying to adopt reasonable practices so that it's just "click the button," or "report bug we find by actually testing things as people might want to install them,"

@bollwyvl bollwyvl closed this Sep 3, 2021
@bollwyvl bollwyvl reopened this Sep 3, 2021
@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I was trying to look for recipes to lint for you, but it appears we have a merge conflict.
Please try to merge or rebase with the base branch to resolve this conflict.

Please ping the 'conda-forge/core' team (using the @ notation in a comment) if you believe this is a bug.

@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipe) and found some lint.

Here's what I've got...

For recipe:

  • Failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your meta.yaml, though. To get a traceback to help figure out what's going on, install conda-smithy and run conda smithy recipe-lint . from the recipe directory.

@bollwyvl bollwyvl changed the title add pip check, use more traditional version selector myst-nb 0.13.0, pip check, update dependencies, add maintainers Sep 3, 2021
@conda-forge-linter
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

Copy link
Contributor

@chrisjsewell chrisjsewell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the requirements should directly replicate those found in the main repo's setup.cfg

Comment on lines +26 to +28
- jupyter-sphinx >=0.3.2,<0.4
- myst-parser >=0.15.2,<0.16
- nbconvert >=5.5,<7

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- jupyter-sphinx >=0.3.2,<0.4
- myst-parser >=0.15.2,<0.16
- nbconvert >=5.5,<7
- jupyter-sphinx ~=0.3.2
- myst-parser ~=0.15.2
- nbconvert >=5.5,<7

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this is how they'd look in the setup.cfg files.

@choldgraf
Copy link

@bollwyvl I certainly agree with that goal! I think the challenge that I have is that, because I don't understand how the conda forge infrastructure works, it is unclear to me whether things are actually as simple as just clicking the button or not haha.

@choldgraf
Copy link

Hey all - I appreciate this discussion around conventions for how to handle dependency specifications for conda-forge recipes, and I appreciate the interest that folks have in ensuring compatibility across the broader conda forge ecosystem.

It sounds like there are reasonable cases to be made on both sides here, and I don't think that there is a single obviously right answer. That being said, I also don't think that this PR is the right place to decide when to change team conventions within executablebooks/ stuff. If there's a strong reason for one convention or another, this should be decided at the project level within the practices that we follow, rather than on an ad-hoc basis in one PR. Otherwise I fear that the standards we follow will start to organically diverge, and it will make the ecosystem harder to keep track of and follow.

@moorepants
Copy link

I don't think anyone is asking to change anything about EBP or any of it's projects. This is a conda-forge project we are communicating on. Seems to me that conda-forge principles and patterns are what govern these repositories. I'm bowing out at this point, as there doesn't seem to be much room for consensus making. Have a great weekend!

P.S. @bollwyvl I was seemingly added to the conda forge jupyter-book team. How do I go about getting off that? As I'd like to stop getting notifications about these recipes.

@choldgraf
Copy link

choldgraf commented Sep 3, 2021

I think this is a challenging aspect of Conda Forge. On the one hand, yes it is in a different organization. On the other hand, as maintainers of the upstream EBP projects we feel an obligation to participate in this space in order to keep the packages up to date.

Realistically it is people following EPB community practices that will be coming here to maintain things. If conda forge recipes go out of date or don't work properly, people are going to come to the EBP repositories to ask for changes, updates, etc. It's not as easy as saying that different practices should be followed just because the repository is in a different GitHub org.

My assumption was that conda forge recipes respect the community practices of the projects that are hosted here. Is that wrong?

and again, my point is not that we should not build consensus, my point is that the place to have consensus is in community discussions around our developer conventions, not in an ad-hoc fashion with a subset of people on one PR in a conda forge recipe.

@mmcky
Copy link
Contributor

mmcky commented Sep 4, 2021

thanks for all these points everyone. I have opened a discussion thread for consideration of these points: executablebooks/meta#481

@bollwyvl bollwyvl closed this Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants