Skip to content

Conversation

@jdblischak
Copy link
Member

Checklist


Closes #12
Closes #14

Blocked by conda-forge/multiscale-spatial-image-feedstock#5

Local rerendering had no effect because recently done in #10. The python_min in the recipe overrides the python_min in .ci_support/.

Explanation of differences from the 0.3.0 pyproject.toml:

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

@giovp
Copy link
Contributor

giovp commented Apr 3, 2025

@conda-forge-admin please rerender

@giovp
Copy link
Contributor

giovp commented Apr 3, 2025

@conda-forge-admin please rerender

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/14249722853. Examine the logs at this URL for more detail.

@giovp
Copy link
Contributor

giovp commented Apr 4, 2025

@conda-forge-admin please rerender

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/14255354543. Examine the logs at this URL for more detail.

@giovp
Copy link
Contributor

giovp commented Apr 4, 2025

alright, I made the call and reverted back to the original pindown of dask. This is finally ready to go!

@giovp
Copy link
Contributor

giovp commented Apr 4, 2025

@conda-forge-admin please rerender

@conda-forge-admin
Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/14269638001. Examine the logs at this URL for more detail.

@giovp giovp merged commit 1cbe783 into conda-forge:main Apr 4, 2025
5 checks passed
@jdblischak jdblischak deleted the 0.3.0-jdb branch April 4, 2025 19:22
build:
noarch: python
script: {{ PYTHON }} -m pip install . -vv
script: {{ PYTHON }} -m pip install . -vv --no-deps --no-build-isolation
Copy link
Member

@LucaMarconato LucaMarconato Apr 21, 2025

Choose a reason for hiding this comment

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

@jdblischak could you please comment on --no-deps and --no-build-isolation?

I see that they are added here in spatialdata-feedstock and in these downstream repositories:

but not in these downstream repositories:

Also the parameters are not used in these upstream repositories:

Copy link
Member

Choose a reason for hiding this comment

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

CC @giovp

Copy link
Member Author

Choose a reason for hiding this comment

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

By default, pip will install any missing dependencies. However, all dependencies should be specified in the recipe and installed by conda. Using the flag --no-deps is a best practice to ensure all the required dependencies are defined in the recipe. It should be added to any recipe that is missing it

Copy link
Member Author

Choose a reason for hiding this comment

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

By default, pip creates a separate build environment, installs the build requirements, and build the package in this separate environment. Thus this completely defeats the purpose of using the conda recipe to specify the build requirements. Any recipe missing --no-build-isolation should have the flag added. It ensures the package is built by the conda packages indicated in the recipe.

Copy link
Member Author

@jdblischak jdblischak Apr 22, 2025

Choose a reason for hiding this comment

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

A good habit when creating a new recipe or updating an existing one is to use grayskull, eg:

grayskull pypi --strict-conda-forge spatialdata
grep script spatialdata/meta.yaml
##   script: {{ PYTHON }} -m pip install . -vv --no-deps --no-build-isolation

which is the officially recommended method to generate conda-forge recipes.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! I will add these parameters to the incoming conda releases.

Copy link
Member

Choose a reason for hiding this comment

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

I tag also the other devs that are listed as maintainers of the repos above @melonora @timtreis @thewtex

Copy link
Member Author

Choose a reason for hiding this comment

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

Quick follow-up to add some more nuance. While the official stance is to add --no-deps --no-build-isolation, there is some debate in the community (conda/grayskull#582, conda/grayskull#584). Essentially in most cases the flags are redundant because conda-build/rattler-build set env vars to accomplish this. However, they are required for recipes with multiple outputs and in the past when conda-build has failed to set the env vars.

Personally, I like including the flags even when they are redundant because they make it more transparent to readers of the recipe that these pip behaviors are being enforced.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for the extra details!

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.

4 participants