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

Add constraints to meta.yaml #212

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

jefsayshi
Copy link

@jefsayshi jefsayshi commented Jul 19, 2024

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.

Resolves #187

Summary of Changes:

@conda-forge-webservices
Copy link
Contributor

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/meta.yaml) and found it was in an excellent condition.

@jefsayshi
Copy link
Author

@conda-forge-admin, please rerender

@jefsayshi
Copy link
Author

@h-vetinari or @lithomas1 is there anything that I need to do to get this merged? I would prefer that it gets merged before another pandas version is released so that the run constraints will be added to it.

@h-vetinari
Copy link
Member

I'm not up to date with the extra-installs that pandas offers. The constraints are probably an OK fix, though it reduces the amount of legal environments that pandas can be installed in.

Better would be to create equivalent outputs here where e.g. pip install pandas[performance] is mapped to conda install pandas-performance. There are many examples of feedstocks that do this, check out for example ray or gymnasium.

But ultimately you'll have to figure this out with the maintainers of this feedstock, which doesn't include me; I'm just helping out here and there.

@jamesmyatt
Copy link

jamesmyatt commented Aug 20, 2024

See also pandas-dev/pandas#52490

@jefsayshi
Copy link
Author

I agree that it will reduce the number of solves, but the ones reduced are only legal until you try to use a pandas function that relies on one of them. My example in issue #187 is sqlalchemy 1.4 and pandas 2.2

This PR will at least align conda-forge with the conda main channel.

While subpackages could end up being the long term solution, there does not seem to be consensus around it yet.

@lithomas1
Copy link
Contributor

I'm taking a break from pandas maintenance this summer.

I'll cycle back to this in a couple weeks, please ping if I don't.
(This looks correct at a first glance, though.)
Thanks.

@lithomas1
Copy link
Contributor

Looking again, I think this just needs a bump of the build number.

(CI is red at the moment, so this wouldn't be mergeable until the next release anyways so maybe the bump is unnecessary and we just wait for the next release)

@jefsayshi
Copy link
Author

I am fine with waiting for the next release, which is why I didn't bump the build number. However, I will defer to your preference, so just let me know what you want me to do.

@jefsayshi
Copy link
Author

@conda-forge-admin, please rerender

@conda-forge-webservices
Copy link
Contributor

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-webservices
Copy link
Contributor

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/meta.yaml) and found it was in an excellent condition.

Copy link
Contributor

@lithomas1 lithomas1 left a comment

Choose a reason for hiding this comment

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

Thanks!

@jorisvandenbossche
Copy link
Member

To what extent do we want to add those constraints?
Because it can be a valid use case to use both pandas and sqlalchemy in the same project, without using the sqlalchemy integration in pandas. And in such a case, it is perfectly fine to use an older version of sqlalchemy than what pandas requires.

This might not be the majority of use cases, but AFAIU adding those constraints would make it impossible to create an environment in such a case?

@jefsayshi
Copy link
Author

It would stop them from using pandas>=2.2 with the older sqlalchemy. They could still use the older versions of pandas. That is a trade-off with this implementation.

And while they may be fine if they don't currently use the sqlalchemy integrations in pandas, as soon as they try to (eg pandas.read_sql) they will run into errors.

@jefsayshi
Copy link
Author

@lithomas1 Can this get merged? I noticed that 2.2.3 was released without it.

@lithomas1
Copy link
Contributor

I think I was happy with the PR the way it is.

@jorisvandenbossche
Can you take another look at this PR? You seemed a little apprehensive about this in your last comment/review.

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.

Add run_constrained for optional dependencies
5 participants