Skip to content

fix: Preserve remote URIs (e.g. s3://) in DaskOfflineStore path #5208

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 30, 2025
Merged

fix: Preserve remote URIs (e.g. s3://) in DaskOfflineStore path #5208

merged 1 commit into from
Jun 30, 2025

Conversation

predictorsSH
Copy link
Contributor

… resolution

What this PR does / why we need it:

Which issue(s) this PR fixes:

Misc

@predictorsSH predictorsSH requested a review from a team as a code owner April 2, 2025 07:41
ntkathole
ntkathole reviewed Apr 2, 2025
View reviewed changes
ntkathole
ntkathole reviewed Apr 3, 2025
View reviewed changes
@ntkathole
Copy link
Member

Please sign the commit so that DCO commit pass

@predictorsSH predictorsSH force-pushed the fix/dask-offline-store-s3-path branch 2 times, most recently from 9356173 to 806d0c4 Compare April 3, 2025 05:24
ntkathole
ntkathole approved these changes Apr 3, 2025
View reviewed changes
Copy link
Member

@ntkathole ntkathole left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@ntkathole
Copy link
Member

@predictorsSH There is 1 more instance in the same file where absolute path is not correctly set up https://github.com/feast-dev/feast/blob/master/sdk/python/feast/infra/offline_stores/dask.py#L472
Can you please apply the same fix for offline_write_batch method ?

@ntkathole ntkathole mentioned this pull request Apr 3, 2025
Open
@predictorsSH
Copy link
Contributor Author

Thanks! I've applied the same fix to offline_write_batch with the same logic.

@franciscojavierarceo franciscojavierarceo changed the title fix(dask): preserve remote URIs (e.g. s3://) in DaskOfflineStore path… fix(dask): Preserve remote URIs (e.g. s3://) in DaskOfflineStore path Apr 4, 2025
@franciscojavierarceo
Copy link
Member

@predictorsSH can you run the linter? make format-python?

@predictorsSH
Copy link
Contributor Author

I've applied the formatting using make format-python

@makSSMZ
Copy link

makSSMZ commented Jun 25, 2025

Hello, is there any updates?

@predictorsSH
Copy link
Contributor Author

No further updates from my side.

@makSSMZ
Copy link

makSSMZ commented Jun 26, 2025

@predictorsSH Sorry, can you tell when the fix will be available? Or will the problem not be fixed at all?

@ntkathole
Copy link
Member

@predictorsSH can you please rebase (and squash, if possible) ? Once CI passed, we can get it merged.

@makSSMZ
Copy link

makSSMZ commented Jun 30, 2025

Can you please tell me the approximate time when the fix will be? We are really looking forward to it. Thank you!

@ntkathole ntkathole force-pushed the fix/dask-offline-store-s3-path branch from 9783043 to 9bab3d2 Compare June 30, 2025 14:22
@ntkathole ntkathole changed the title fix(dask): Preserve remote URIs (e.g. s3://) in DaskOfflineStore path fix: Preserve remote URIs (e.g. s3://) in DaskOfflineStore path Jun 30, 2025
@ntkathole ntkathole enabled auto-merge (rebase) June 30, 2025 14:32
@ntkathole ntkathole merged commit 2561cfc into feast-dev:master Jun 30, 2025
16 of 17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ntkathole ntkathole ntkathole approved these changes

Assignees
No one assigned
Labels
ok-to-test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@predictorsSH @ntkathole @franciscojavierarceo @makSSMZ @HaoXuAI