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

Fix: Upgrade stow to fix listing error files #6159

Merged
merged 4 commits into from
Jan 11, 2025

Conversation

fg91
Copy link
Member

@fg91 fg91 commented Jan 10, 2025

Why are the changes needed?

For RFC #5598, flytepropeller was given the ability to list error files in the so-called raw output prefix bucket of an execution with the goal of identifying which worker pod in a failed distributed task experienced the first error.

For this purpose, the StowStore in flytestdlib was given a List function.

For google cloud storage buckets, the listing and subsequent access of the error files currently does not work: When listing a bucket gs://some-bucket/..., one receives items in the form google://storage.googleapis.com/download/storage/v1/b/some-bucket/... which then cannot be found by the stow store for the gs:// prefix.

This was fixed in flyteorg/stow#17. This PR upgrades the stow dependency

Summary by Bito

Dependency upgrade of stow from v0.3.10 to v0.3.11 across Flyte components to resolve GCS bucket listing issues. The update fixes incompatible URL format problems in Google Cloud Storage buckets. This change supports RFC #5598 implementation for improved failure detection in distributed tasks.

Unit tests added: False

Estimated effort to review (1-5, lower is better): 3

@fg91 fg91 requested a review from eapolinario January 10, 2025 17:36
@fg91 fg91 self-assigned this Jan 10, 2025
@fg91 fg91 added the dependencies Pull requests that update a dependency file label Jan 10, 2025
@flyte-bot
Copy link
Collaborator

flyte-bot commented Jan 10, 2025

Code Review Agent Run Status

  • Limitations and other issues: ❌ Failure - We encountered technical difficulties while attempting to generate code feedback. Please try again or contact [email protected].

@fg91 fg91 added the bug Something isn't working label Jan 10, 2025
Copy link

codecov bot commented Jan 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 37.02%. Comparing base (e21a6ad) to head (4f3067c).
Report is 23 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6159      +/-   ##
==========================================
+ Coverage   36.98%   37.02%   +0.04%     
==========================================
  Files        1318     1317       -1     
  Lines      132449   132523      +74     
==========================================
+ Hits        48984    49065      +81     
  Misses      79212    79212              
+ Partials     4253     4246       -7     
Flag Coverage Δ
unittests-datacatalog 51.58% <ø> (ø)
unittests-flyteadmin 54.25% <ø> (+0.21%) ⬆️
unittests-flytecopilot 30.99% <ø> (ø)
unittests-flytectl 62.29% <ø> (ø)
unittests-flyteidl 7.23% <ø> (+<0.01%) ⬆️
unittests-flyteplugins 53.85% <ø> (-0.01%) ⬇️
unittests-flytepropeller 42.64% <ø> (+0.03%) ⬆️
unittests-flytestdlib 55.17% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

fg91 added 2 commits January 10, 2025 17:58
Signed-off-by: Fabio Graetz <[email protected]>
Signed-off-by: Fabio Graetz <[email protected]>
@flyte-bot
Copy link
Collaborator

flyte-bot commented Jan 10, 2025

Code Review Agent Run #19512d

Actionable Suggestions - 0
Review Details
  • Files reviewed - 18 · Commit Range: 3b2b0cd..4f3067c
    • datacatalog/go.mod
    • datacatalog/go.sum
    • flyteadmin/go.mod
    • flyteadmin/go.sum
    • flytecopilot/go.mod
    • flytecopilot/go.sum
    • flytectl/go.mod
    • flytectl/go.sum
    • flyteidl/go.mod
    • flyteidl/go.sum
    • flyteplugins/go.mod
    • flyteplugins/go.sum
    • flytepropeller/go.mod
    • flytepropeller/go.sum
    • flytestdlib/go.mod
    • flytestdlib/go.sum
    • go.mod
    • go.sum
  • Files skipped - 0
  • Tools
    • Golangci-lint (Linter) - ✖︎ Failed
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • SNYK (Security Vulnerability) - ✔︎ Successful
    • OWASP (Security Vulnerability) - ✔︎ Successful
    • GOVULNCHECK (Security Vulnerability) - ✖︎ Failed

AI Code Review powered by Bito Logo

@flyte-bot
Copy link
Collaborator

Changelist by Bito

This pull request implements the following key changes.

Key Change Files Impacted
Bug Fix - Upgrade Stow Dependency to Fix GCS Listing Issue

go.mod - Update stow from v0.3.10 to v0.3.11

go.sum - Update stow dependency checksums

go.mod - Update stow from v0.3.10 to v0.3.11

go.sum - Update stow dependency checksums

go.mod - Update stow from v0.3.10 to v0.3.11

go.sum - Update stow dependency checksums

go.mod - Update stow from v0.3.10 to v0.3.11

go.sum - Update stow dependency checksums

go.mod - Update stow and add new dependencies

go.sum - Update dependency checksums and add new entries

go.mod - Update stow from v0.3.10 to v0.3.11

go.sum - Update stow dependency checksums

go.mod - Update stow from v0.3.10 to v0.3.11

go.sum - Update stow dependency checksums

go.mod - Update stow from v0.3.10 to v0.3.11

go.sum - Update stow dependency checksums

go.mod - Update stow from v0.3.10 to v0.3.11

go.sum - Update stow dependency checksums

Copy link
Member

@pingsutw pingsutw left a comment

Choose a reason for hiding this comment

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

Thank you!

@fg91 fg91 merged commit b8fb68d into master Jan 11, 2025
55 of 56 checks passed
@fg91 fg91 deleted the fg91/fix/propeller-upgrade-stow branch January 11, 2025 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants