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

Constraint the full fsspec family #2087

Merged
merged 1 commit into from
Jan 3, 2024
Merged

Conversation

honnix
Copy link
Member

@honnix honnix commented Jan 3, 2024

Tracking issue

Closes flyteorg/flyte#4665

Why are the changes needed?

Described in flyteorg/flyte#4665

What changes were proposed in this pull request?

Use the same constraint for all packages from fsspec family

How was this patch tested?

Through CI probably.

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

Copy link

codecov bot commented Jan 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (6f613e7) 85.76% compared to head (890751e) 85.76%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2087   +/-   ##
=======================================
  Coverage   85.76%   85.76%           
=======================================
  Files         313      313           
  Lines       23426    23426           
  Branches     3510     3510           
=======================================
  Hits        20092    20092           
  Misses       2727     2727           
  Partials      607      607           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Hongxin Liang <honnix@users.noreply.github.com>
@honnix honnix merged commit 9bf7afa into master Jan 3, 2024
84 checks passed
@mark-thm
Copy link

mark-thm commented Jan 4, 2024

@honnix curious if you have a sense of what it'd take to increase the upper range here? I tried (and failed) to bump it for flyte but couldn't decipher the windows errors.

@honnix honnix deleted the honnix/fsspec-version-fix branch January 4, 2024 16:17
@honnix
Copy link
Member Author

honnix commented Jan 4, 2024

@mark-thm I think you will need to bump all those packages to a matching version. They are usually matched on "20xx.x" level but there seems to be exceptions.

@Tom-Newton
Copy link
Contributor

Tom-Newton commented Jan 26, 2024

I would be very interested in bumping the upper range. There is actually no version of adlfs in the range which doesn't suffer from this bug. The latest working version is 2022.11.2 and its still broken on main. fsspec/adlfs#454 should fix it.

It seems to me like its largely the user's responsibility to ensure that fsspec packages are aligned. I don't see how it can be done on a flyte level without restricting to a very tight range.

@honnix
Copy link
Member Author

honnix commented Jan 26, 2024

Yeah, the fsspec package versions are tricky.

@Tom-Newton
Copy link
Contributor

Tom-Newton commented Jan 26, 2024

Personally I think I would be in favour of removing all the upper limits, so that users have control.

I'm definitely going to want to use a new version of adlfs once fsspec/adlfs#389 gets resolved.

@Tom-Newton
Copy link
Contributor

Tom-Newton commented Jan 29, 2024

I just discovered that each version of adlfs, gcsfs, s3fs put constraints on the version of fsspec they are compatible with, so there is no need to do this at the flytekit level. I think flytekit should allow unbounded new versions of all these libraries. For people who have monorepos, highly restrictive dependencies like this PR introduces are a problem.

$ pip install fsspec==2023.12.2
Collecting fsspec==2023.12.2
  Using cached fsspec-2023.12.2-py3-none-any.whl (168 kB)
ERROR: s3fs 2023.12.1 has requirement fsspec==2023.12.1, but you'll have fsspec 2023.12.2 which is incompatible.
ERROR: gcsfs 2023.12.1 has requirement fsspec==2023.12.1, but you'll have fsspec 2023.12.2 which is incompatible.
Installing collected packages: fsspec

I haven't found where the constraint is in the code for gcsfs and s3fs, but for adlfs its here
https://github.com/fsspec/adlfs/blob/18ea349d8a286f8bbeb8055127a9d58fe4cde0d9/pyproject.toml#L30

@Tom-Newton
Copy link
Contributor

Tom-Newton commented Feb 14, 2024

It looks like #2143 has addressed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants