GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fix: Preserve remote URIs (e.g. s3://) in DaskOfflineStore path by predictorsSH · Pull Request #5208 · feast-dev/feast

Conversation

@predictorsSH

@predictorsSH predictorsSH commented Apr 2, 2025

Copy link
Copy Markdown
Contributor

… resolution

What this PR does / why we need it:

Which issue(s) this PR fixes:

Misc

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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
Comment thread sdk/python/feast/infra/offline_stores/dask.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
ntkathole
ntkathole reviewed Apr 3, 2025
View reviewed changes
Comment thread sdk/python/feast/infra/offline_stores/dask.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@ntkathole

ntkathole commented Apr 3, 2025

Copy link
Copy Markdown
Member

Please sign the commit so that DCO commit pass
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

@ntkathole ntkathole left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ntkathole

ntkathole commented Apr 3, 2025

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 ?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ntkathole ntkathole mentioned this pull request Apr 3, 2025
Newest updates break DaskOfflineStore with S3 parquets #4753
Closed
@predictorsSH

predictorsSH commented Apr 3, 2025

Copy link
Copy Markdown
Contributor Author

Thanks! I've applied the same fix to offline_write_batch with the same logic.
ntkathole reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

franciscojavierarceo commented Apr 4, 2025

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@predictorsSH

predictorsSH commented Apr 6, 2025

Copy link
Copy Markdown
Contributor Author

I've applied the formatting using make format-python
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@haoxu0 haoxu0 added the ok-to-test label Apr 10, 2025
@makSSMZ

makSSMZ commented Jun 25, 2025

Copy link
Copy Markdown

Hello, is there any updates?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@predictorsSH

predictorsSH commented Jun 26, 2025

Copy link
Copy Markdown
Contributor Author

No further updates from my side.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ntkathole

ntkathole commented Jun 27, 2025

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@predictorsSH @ntkathole
fix(dask): preserve remote URIs (e.g. s3://) in DaskOfflineStore path…
9bab3d2 
… resolution

Signed-off-by: sanghyun <[email protected]>
@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

Reviewers

@ntkathole ntkathole ntkathole approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

ok-to-test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@predictorsSH @ntkathole @franciscojavierarceo @makSSMZ @haoxu0
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.