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

ci: include optional dependencies for Polars backend in ibis-framework install #3494

Merged
merged 3 commits into from
Jul 23, 2024

Conversation

MarcoGorelli
Copy link
Contributor

I'm seeing a CI failure in the downstream tests: https://github.com/narwhals-dev/narwhals/actions/runs/10055785067/job/27793203912?pr=587

Thanks for contributing to Altair!

Please follow these guidelines when submitting your PR:

  • Describe the purpose of the changes in PR, so that it is easy to understand the implication of the suggested changes.
  • Include unit tests and documentation for new features
  • Ensure that the title is a concise semantic commit message (e.g. "feat: Add embed_options to charts").
    • Append ! if the change is breaking (e.g. "fix!: Raise error when embed_options is None")

@mattijn
Copy link
Contributor

mattijn commented Jul 23, 2024

Seems Altair is not including the pyarrow-hotfix package, from what I see what is included within ibis-framework[polars]:

https://github.com/ibis-project/ibis/blob/aa1eff703a0b53899b5e70e1a08ac23ee23e2a2c/pyproject.toml#L165C46-L165C60

polars = ["polars", "packaging", "pyarrow", "pyarrow-hotfix"]

And what is not included within Altair pyproject.toml:

altair/pyproject.toml

Lines 57 to 83 in f2ac0a1

all = [
"vega_datasets>=0.9.0",
"vl-convert-python>=1.5.0",
"pandas>=0.25.3",
"numpy<2.0.0",
"pyarrow>=11",
"vegafusion[embed]>=1.6.6",
"anywidget>=0.9.0",
"altair_tiles>=0.3.0"
]
dev = [
"hatch",
"ruff>=0.5.3",
"ibis-framework",
"ipython",
"pandas>=0.25.3",
"pytest",
"pytest-cov",
"pytest-xdist[psutil]~=3.5",
"m2r",
"mypy",
"pandas-stubs",
"types-jsonschema",
"types-setuptools",
"geopandas",
"polars>=0.20.3",
]

@mattijn
Copy link
Contributor

mattijn commented Jul 23, 2024

This hotfix for pyarrow is required until our minimum supported pyarrow version is 14.0.1 as is documented here: https://github.com/pitrou/pyarrow-hotfix#readme

We generally recommend upgrading to PyArrow 14.0.1 or later, but if you cannot upgrade, this package disables the vulnerability on older versions.

But since you have pyarrow==17.0.0 in your CI run. It should not be needed anymore anymore within Ibis.

It is better if Ibis does something like this within their pyproject file:

"pyarrow_hotfix; pyarrow_version<\"14.0.1\"",

Edit: above line doesn't work, see next comment.

And reflect this within their code as an optional dependency based on the version of pyarrow.

For now, it is probably the best to do both this PR including adding this line to our all.

required until our minimum supported version of pyarrow is 14.0.1
@mattijn
Copy link
Contributor

mattijn commented Jul 23, 2024

Reverting my last commit since what I tried doesn't work.
See here what is allowed within hatch for environmental markers: https://hatch.pypa.io/dev/config/dependency/#environment-markers.

@mattijn
Copy link
Contributor

mattijn commented Jul 23, 2024

Created a separate issue for including pyarrow hotfix within altair itself, #3495

@dangotbanned
Copy link
Member

dangotbanned commented Jul 23, 2024

Just arriving here after updating these:

Hi everyone

@dangotbanned
Copy link
Member

dangotbanned commented Jul 23, 2024

Would reusing whatever solution this PR ends on be the simplest?

Related issue

@MarcoGorelli MarcoGorelli marked this pull request as ready for review July 23, 2024 12:21
@MarcoGorelli
Copy link
Contributor Author

thanks both!

cool, so I'll leave this PR as it is, and pyarrow-hotfix in Altair itself can be dealt with separately?

Created a separate issue for including pyarrow hotfix within altair itself, #3494

#3495 ?

@dangotbanned
Copy link
Member

Created a separate issue for including pyarrow hotfix within altair itself, #3494

#3495 ?

Yeah this one rattled me as well

@mattijn
Copy link
Contributor

mattijn commented Jul 23, 2024

Circular references forward😵‍💫. Merging this, thanks!

@mattijn mattijn merged commit 0f16abb into vega:main Jul 23, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants