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

Update list of runtime dependencies in recipe/meta.yml #71

Closed

Conversation

amotl
Copy link
Member

@amotl amotl commented Nov 28, 2022

This patch just follows the suggestions by regro-cf-autotick-bot about packages found by source code inspection but not in the meta.yaml vs. packages found in the meta.yaml but not found by source code inspection.

It addresses our discussion at:

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

@xylar
Copy link
Contributor

xylar commented Nov 28, 2022

@amotl, I wouldn't make these changes even though the bot suggested them. It is important that the dependencies are the same as in the original package:
https://github.com/earthobservations/wetterdienst/blob/v0.48.0/pyproject.toml#L91-L117
If there are missing dependencies, you need to report them to the developers rather than just changing them in the conda-forge recipe. Sometimes the "missing" dependencies are only in tests or code for development, not the package for general use.

@amotl amotl mentioned this pull request Nov 28, 2022
3 tasks
@amotl
Copy link
Member Author

amotl commented Nov 28, 2022

Dear @xylar,

thanks for looking into this. Indeed, most of the added dependencies are optional dependencies 1. Is there any way to specify them within the Anaconda metadata file?

Otherwise, is the general recommendation to not include them into the baseline meta.yml, and let the user select additional/optional packages instead?

With kind regards,
Andreas.

Footnotes

  1. https://setuptools.pypa.io/en/latest/userguide/dependency_management.html#optional-dependencies

@xylar
Copy link
Contributor

xylar commented Nov 28, 2022

@amotl, yes, there is a way to handle optional dependencies. It's a little complicated. I'll send you an example tomorrow and we can discuss further.

@amotl amotl requested a review from xylar as a code owner November 28, 2022 22:00
@amotl
Copy link
Member Author

amotl commented Nov 28, 2022

Thanks. I've just added e7e2e94 to this patch, which updates the list of dependencies to be more sensible as a default choice. If @gutzbenj agrees, I think it will be good to go, without the need to overengineer things.

@xylar
Copy link
Contributor

xylar commented Nov 28, 2022

Okay here's the example
https://github.com/conda-forge/airflow-feedstock/blob/main/recipe/meta.yaml
The extras are separate packages that include the base package and are of the form <main>-with-<extra>. This will only work well if extras are grouped together. We don't want a gagillion additional packages. It's only worth it if users really want these extras.

@amotl
Copy link
Member Author

amotl commented Nov 28, 2022

I see, thank you. Based on this information, I think it is not worth to add additional maintenance overhead to the wetterdienst-feedstock package.

How would we proceed with this patch, then? Cherry-pick it into #70? I will be happy about any advises.

@xylar
Copy link
Contributor

xylar commented Nov 28, 2022

I will look at this tomorrow but even if these are the right dependencies, they are missing version constraints from the original package and they are very important to include. I will make suggestions to #70 and ask for your feedback.

This just follows the suggestions by `regro-cf-autotick-bot` about
packages found by source code inspection but not in the meta.yaml vs.
packages found in the meta.yaml but not found by source code inspection.
- Don't install FastAPI and Dash/Plotly by default
- Install dependencies needed by "interpolation" feature
@amotl amotl force-pushed the amo/update-dependencies branch from e7e2e94 to 2bee959 Compare November 28, 2022 22:15
@amotl
Copy link
Member Author

amotl commented Nov 28, 2022

I will make suggestions to #70 and ask for your feedback.

Apologies. I've just merged the release PR, in order to get the release out of the door, without mixing in unrelated changes.

I will look at this tomorrow but even if these are the right dependencies, they are missing version constraints from the original package and they are very important to include.

Ah - version pinning for Anaconda? I will be happy to learn more about writing a better meta.yml. Feel free to submit your suggestions as a patch right away. Thank you already!

@xylar
Copy link
Contributor

xylar commented Nov 28, 2022

@amotl, hmm, I wish you hadn't merged that yet. The dependencies are not correct in #70 and need to be fixed. That is a lot harder to do after the fact. It is also nice to fix at a new release.

Copy link
Contributor

@xylar xylar left a comment

Choose a reason for hiding this comment

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

Please don't merge this until I have a chance to fix several things.

@xylar xylar mentioned this pull request Nov 29, 2022
4 tasks
Comment on lines +51 to +53
- utm
- shapely
- pytz
Copy link
Member Author

Choose a reason for hiding this comment

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

@xylar: What about adding those to #76?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy to do that. These are among the "optional" dependencies. So we should make a note of that. Some users may prefer only to have the minimal package but you will know your users better than I do.

@amotl
Copy link
Member Author

amotl commented Nov 29, 2022

Closing this in favor of #76. Thank you very much, @xylar!

@amotl amotl closed this Nov 29, 2022
@amotl amotl deleted the amo/update-dependencies branch November 29, 2022 09:46
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