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 altair all package corresponding to pip "extras" group #53

Merged
merged 14 commits into from
Mar 18, 2024

Conversation

joelostblom
Copy link
Contributor

@joelostblom joelostblom commented Mar 11, 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

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.

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

For recipe:

  • It looks like the 'altair-all' output doesn't have any tests.

Copy link
Contributor Author

@joelostblom joelostblom left a comment

Choose a reason for hiding this comment

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

@jonmmease @binste @mattijn This is an attempt to create a conda package for the new altair['all'] pip extras group that we added in vega/altair#3354 vega/altair#2818.

I used the recipes of matplotlib, seaborn, and opencv as a template, together with info from this and this discussion.

In addition to my questions in the inline comments, I noticed that seaborn took the approaches of creating a -base package for the barebones install and then have the conda package that just contains the package name correspond to the full install with the [all] group. matplotlib does something similar. We could consider doing this as well, although I personally think that it is clearer if we use altair-all to correspond to altair[all].

If we create altair-all here, we should decide where anywidget and vega-datasets goes. These are already part of the base altair package in conda (probably for testing purposes), but we could break them out into the altair-all package instead if we want. I vote doing whatever is the simplest, which might be to leave them where they are now, but open to hear other thoughts.

Note that although I could run build-locally.py without issues wit hthis recipe, I couldn't find a way to install and test the created packages.

recipe/meta.yaml Outdated
@@ -11,46 +11,64 @@ source:
sha256: 2ad7f0c8010ebbc46319cc30febfb8e59ccf84969a201541c207bc3a4fa6cf81

build:
number: 0
number: 1
noarch: python
script: {{ PYTHON }} -m pip install . -vv
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason {{ PYTHON }} did not work when running ./build-locally.py and I had to replace it with just python, but since it never caused issues on conda-forge, I'm leaving it as is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this seems to be the reason for the failing tests as well... @ocefpaf Do you know why this is happening since the docs mention that we should use {{ PYTHON }}?

image

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this is still a problem but most of the time, issues like that one, are solved by re-rendering the feedstock.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @ocefpaf , I just tried re-rendering and I'm still seeing the same error in the pipelines https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=895875&view=results

Comment on lines +42 to +43
- vega_datasets >=0.9.0
- anywidget >=0.9.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These were added previously, vega_datasets since 6 years ago, and anywidget since 5.1

recipe/meta.yaml Outdated
- pyarrow >=11
- vegafusion-python-embed >=1.5.0
- {{ pin_subpackage('altair', exact=True) }}
# - altair_tiles >=0.3.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't find a way to install from pip in the conda recipe as it seems like the approach used in local conda envs (pip:) does not work. If anyone knows how to install altair_tiles from pip as part of this setup file please chime in (or if you feel like creating a conda package @binste).

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 very much @joelostblom for this PR! I've never created a conda package before so appreciate it that you started this :)

I might have some time on the weekend to try to create a conda package as well for altair_tiles so that we can include it here.

- vl-convert-python >=1.1.0
- pyarrow >=11
- vegafusion-python-embed >=1.5.0
- {{ pin_subpackage('altair', exact=True) }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied this from the seboarn recipe and saw that matplotlib has something similar although slightly different. Let me know if this looks reasonable

Copy link
Contributor

Choose a reason for hiding this comment

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

nice, I hadn't seen this before

Comment on lines +83 to +86
- joelostblom
- jonmmease
- binste
- mattijn
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added everyone listed on PyPI as a package maintainer here so that we get tagged in the PRs made to this repo. Let me know if you want you name removed

recipe/meta.yaml Show resolved Hide resolved
Copy link
Contributor

@jonmmease jonmmease left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this @joelostblom!

recipe/meta.yaml Show resolved Hide resolved
- vl-convert-python >=1.1.0
- pyarrow >=11
- vegafusion-python-embed >=1.5.0
- {{ pin_subpackage('altair', exact=True) }}
Copy link
Contributor

Choose a reason for hiding this comment

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

nice, I hadn't seen this before

recipe/meta.yaml Outdated
imports:
- altair
- vegafusion
- vl-convert-python
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will need to be vl_convert to match the import rather than the package name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, fixed!

@joelostblom
Copy link
Contributor Author

@conda-forge-admin, please rerender

conda-forge-webservices[bot] and others added 3 commits March 14, 2024 15:23
@conda-forge-webservices
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.

@ocefpaf
Copy link
Member

ocefpaf commented Mar 14, 2024

The main issue here was b/c multiple outputs are a different beast and we need a .sh script for them.

@joelostblom
Copy link
Contributor Author

Super, thanks for your help @ocefpaf ! We will wait to have conda package for the altair-tiles dependency and then I will mark this as ready for review.

@binste
Copy link
Member

binste commented Mar 16, 2024

I've now created a recipe for altair_tiles and am waiting for a review at conda-forge/staged-recipes#25760. I'll let you know once altair_tiles is in conda-forge :)

@binste
Copy link
Member

binste commented Mar 17, 2024

altair_tiles is available on conda-forge: https://anaconda.org/conda-forge/altair_tiles I installed and tested it, looks good to me! I'll add the conda install commands to the altair_tiles docs soon

@conda-forge-webservices
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.

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

For recipe:

  • It looks like the 'altair-all' output doesn't have any tests.

@joelostblom
Copy link
Contributor Author

@conda-forge-admin, please rerender

@joelostblom joelostblom marked this pull request as ready for review March 17, 2024 17:08
@joelostblom
Copy link
Contributor Author

joelostblom commented Mar 17, 2024

Thanks @binste! I uncommented those lines, marked this as ready for review and -rerequested reviews from everyone who commented so far (I'm unsure about the checkbox for the packaging of a license file; do we need to manually upload the Altair license file as part of this PR?).

@ocefpaf ocefpaf merged commit b3e4e97 into conda-forge:main Mar 18, 2024
3 checks passed
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