Skip to content

Add mindeps build with all optional dependencies#10161

Merged
jrbourbeau merged 35 commits intodask:mainfrom
charlesbluca:mindeps-optional
Apr 19, 2023
Merged

Add mindeps build with all optional dependencies#10161
jrbourbeau merged 35 commits intodask:mainfrom
charlesbluca:mindeps-optional

Conversation

@charlesbluca
Copy link
Member

@charlesbluca charlesbluca commented Apr 7, 2023

Adds a build running the test suite with all the optional dependencies used through Dask pinned to the lowest versions I could reasonably get working.

cc @jrbourbeau

@charlesbluca charlesbluca marked this pull request as ready for review April 8, 2023 04:17
Copy link
Member Author

@charlesbluca charlesbluca left a comment

Choose a reason for hiding this comment

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

Most of the test failures have been resolved at this point, the only ones remaining I've left because I'm not sure if we'd prefer bumping the minimum supported version or just patching the tests with some compat code:

  • the fastparquet failures are due to lack of support for reading/writing statistics & nullable dtypes
  • the zarr failures are due to zarr.Array expecting an initialized store rather than a string or path-like

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks @charlesbluca!

- tblib=1.5.0
- tiledb-py=0.8.1
- xxhash=0.6.5
- zarr=2.4.0
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind taking a look at #10137, since it's related?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup! Left a comment there, in general looks like our options are either:

  • setting the min version to 2.4.0 and modifying tests to explicitly instantiate zarr stores if we're using <2.12.0
  • setting the min version to 2.12.0 (~9 months old) like you've done in your PR

Do you have a preference here?

Copy link
Member

Choose a reason for hiding this comment

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

I have a preference for setting 2.12.0 as the minimum

@github-actions github-actions bot added the documentation Improve or add to documentation label Apr 11, 2023
Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks @charlesbluca. This is looking really close

+-----------------+-------------+--------------------------------------------------------------------------------------------------------+
| fastparquet | >=0.8.2 | Storing and reading data from Apache Parquet files |
+-----------------+-------------+--------------------------------------------------------------------------------------------------------+
| gcsfs | >=0.4.0 | Storing and reading data located in Google Cloud Storage |
Copy link
Member Author

Choose a reason for hiding this comment

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

Worth noting here that gcsfs doesn't seem to be getting tested anywhere, so this min version is likely outdated - might be worth it to add some tests in the medium term, but I can see if we can bump this version based on our new fsspec min

Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for all your work here @charlesbluca

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improve or add to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add new mindeps build with all optional dependencies

2 participants