Skip to content

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

Merged
ntkathole merged 1 commit into
feast-dev:masterfrom
predictorsSH:fix/dask-offline-store-s3-path
Jun 30, 2025
Merged

ntkathole merged 1 commit into
feast-dev:masterfrom
predictorsSH:fix/dask-offline-store-s3-path

Conversation

@predictorsSH

Copy link
Copy Markdown
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

Copy link
Copy Markdown
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 left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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 ?

@predictorsSH

Copy link
Copy Markdown
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
Copy Markdown
Member

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

@predictorsSH

Copy link
Copy Markdown
Contributor Author

I've applied the formatting using make format-python

@makSSMZ

makSSMZ commented Jun 25, 2025

Copy link
Copy Markdown

Hello, is there any updates?

@predictorsSH

Copy link
Copy Markdown
Contributor Author

No further updates from my side.

@makSSMZ

makSSMZ commented Jun 26, 2025

Copy link
Copy Markdown

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

@ntkathole

Copy link
Copy Markdown
Member

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

@makSSMZ

makSSMZ commented Jun 30, 2025

Copy link
Copy Markdown

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
@predictorsSH
predictorsSH deleted the fix/dask-offline-store-s3-path branch October 23, 2026 06:44
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.

5 participants