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

jupyter-ai v2.28.1 (removes post-link script) #47

Closed
wants to merge 3 commits into from

Conversation

dlqqq
Copy link
Member

@dlqqq dlqqq commented Sep 20, 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.

@conda-forge-webservices
Copy link
Contributor

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/meta.yaml) and found some lint.

Here's what I've got...

For recipe/meta.yaml:

  • requirements: run: faiss-cpu>=1.8.0,<2 must contain a space between the name and the pin, i.e. faiss-cpu >=1.8.0,<2

@dlqqq
Copy link
Member Author

dlqqq commented Sep 20, 2024

@conda-forge-admin, please rerender

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

@dlqqq
Copy link
Member Author

dlqqq commented Sep 20, 2024

Hm, looks like pip check is failing because we declare faiss-cpu as a dependency in pyproject.toml, since that is the easiest way to install faiss via PyPI. However,installing faiss-cpu via Conda Forge only installs the faiss package and does not include a wheel for faiss-cpu, which results in pip check failing. Probably won't have time to look into this, but I'm documenting this error here for now.

@dlqqq dlqqq force-pushed the drop-post-link-script branch from bba0524 to 953a8fc Compare November 8, 2024 19:54
@conda-forge-admin
Copy link
Contributor

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/meta.yaml) and found some lint.

Here's what I've got...

For recipe/meta.yaml:

  • requirements: run: faiss-cpu>=1.8.0,<2 must contain a space between the name and the pin, i.e. faiss-cpu >=1.8.0,<2

For recipe/meta.yaml:

  • noarch: python recipes should almost always follow the syntax in our documentation. For the host section of the recipe, you should almost always use python {{ python_min }} for the python entry. For the run section of the recipe, you should almost always use python >={{ python_min }} for the python entry. For the test.requires section of the recipe, you should almost always use python {{ python_min }} for the python entry. You may need to override the python_min variable in the conda_build_config.yaml/variants.yaml if the package requires a newer Python version than the currently supported minimum version on conda-forge.
  • PyPI default URL is now pypi.org, and not pypi.io. You may want to update the default source url.

@dlqqq
Copy link
Member Author

dlqqq commented Nov 8, 2024

This probably can be fixed by simply removing the pip check call in the recipe, but this may cause downstream issues in CI pipelines that install Jupyter AI via Conda Forge and run pip check. I'll see if there's a safer solution to this.

@dlqqq
Copy link
Member Author

dlqqq commented Nov 8, 2024

There is an existing issue for pip check failing when a dependency on PyPI is provided via Conda, but not installed as a Python package: pypa/pip#11157

However, progress had halted months ago. It may be better just to remove pip check from the recipe, as recommended here by a member of PyPA: pypa/pip#11159 (comment)

@dlqqq
Copy link
Member Author

dlqqq commented Nov 8, 2024

I've taken all conceivable user concerns about this issue into account, and have reached a conclusion on the best path forward.

It seems best to remove pip check and install faiss-cpu directly from Conda Forge to fix Conda Forge installs for Jupyter AI. This will break pip check when Jupyter AI is installed via Conda Forge, but this is an existing issue for other Conda packages and has no clear path to remediation currently. While it's unfortunate we have to remove this check, removing this check is a better alternative than leaving Jupyter AI unavailable on Conda Forge.

Users should open an issue on this feedstock if they have a use-case where this is unacceptable. If this occurs and we find a better solution to this issue, we can re-build the affected releases of Jupyter AI on Conda Forge.

@dlqqq
Copy link
Member Author

dlqqq commented Nov 8, 2024

Technically, this requires a patch release in Jupyter AI first to fix the version specifier declared for faiss-cpu, which currently is set to faiss-cpu<=1.8.0. I'll do that, cut the patch release on PyPI, then update & merge this PR to cut the patch release on Conda Forge.

@dlqqq dlqqq changed the title Remove post-link script and install faiss-cpu via Conda jupyter-ai v2.28.1 (removes post-link script) Nov 11, 2024
@dlqqq dlqqq marked this pull request as ready for review November 11, 2024 20:33
@conda-forge-admin
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.

I do have some suggestions for making it better though...

For recipe/meta.yaml:

  • ℹ️ noarch: python recipes should usually follow the syntax in our documentation for specifying the Python version.
    • For the host section of the recipe, you should usually use python {{ python_min }} for the python entry.
    • For the run section of the recipe, you should usually use python >={{ python_min }} for the python entry.
    • For the test.requires section of the recipe, you should usually use python {{ python_min }} for the python entry.
    • If the package requires a newer Python version than the currently supported minimum version on conda-forge, you can override the python_min variable by adding a Jinja2 set statement at the top of your recipe (or using an equivalent context variable for v1 recipes).
  • ℹ️ PyPI default URL is now pypi.org, and not pypi.io. You may want to update the default source url.

@dlqqq dlqqq closed this Nov 11, 2024
@dlqqq dlqqq reopened this Nov 11, 2024
@dlqqq
Copy link
Member Author

dlqqq commented Nov 11, 2024

@conda-forge-admin please rerun bot

@conda-forge-webservices conda-forge-webservices bot added the bot-rerun Instruct the bot to retry the PR label Nov 11, 2024
@dlqqq dlqqq closed this Nov 11, 2024
@dlqqq dlqqq reopened this Nov 11, 2024
@dlqqq
Copy link
Member Author

dlqqq commented Nov 11, 2024

Unfortunately the bug tracked by #57 is happening again. I'll have to open a new PR.

@dlqqq dlqqq closed this Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot-rerun Instruct the bot to retry the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants