Fix local card storage root resolution#3301
Open
arjunxplorer wants to merge 2 commits into
Open
Conversation
Keep local and spin cards under the resolved artifact datastore root when no explicit card root is configured, ensuring card writers and readers use the same location.
Contributor
Greptile SummaryThis PR fixes local card storage root resolution. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "Handle empty local card datastore roots" | Re-trigger Greptile |
Treat empty local and spin datastore roots as missing so card root resolution preserves the legacy lookup behavior for older metadata. Co-authored-by: Cursor <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
PR Type
Summary
Keep local and spin cards under the resolved artifact datastore root when no explicit card root is configured. Also ensure an explicit
METAFLOW_CARD_LOCALROOTis returned and used correctly.Issue
Fixes #2139
Reproduction
Runtime: Local datastore with a separate datastore sysroot, reproducing the storage configuration used with local Kubernetes.
Commands to run:
Where evidence shows up: The filesystem card location and the
card listoutput exercised by the integration test.Before (error / log snippet)
The card HTML was written under
<cwd>/.metaflow/mf.cardsinstead of the configured local datastore root.The explicit-root unit test also failed because
get_storage_root()returnedNonewhenCARD_LOCALROOTwas configured.After (evidence that fix works)
The card is written under
<sysroot>/.metaflow/mf.cards, no stray<cwd>/.metaflowdirectory is created, andcard listfinds the card.Root Cause
Artifact storage resolves
METAFLOW_DATASTORE_SYSROOT_LOCAL, but card storage independently searched upward from the current working directory. Consequently, artifacts and cards could use different roots.Additionally, when
CARD_LOCALROOTwas configured, the local/spin branch assigned it to a variable but did not return it, causing the method to returnNone.Why This Fix Is Correct
Local and spin card roots now follow a single resolution order:
CARD_LOCALROOT.mf.cardsto the already-resolved artifact datastore root.The card writer and reader both use this resolution logic, restoring the invariant that cards are read from the same location where they were written.
Failure Modes Considered
CARD_LOCALROOTcontinues to take precedence over the artifact datastore root.Tests
Local verification:
CI remains unchecked until the pull request checks complete.
Non-Goals
This change does not alter cloud card storage behavior, change card file layout, or remove the existing local fallback behavior.
AI Tool Usage
Cursor was used to assist with issue investigation, implementation, comments, and regression tests. I reviewed the changes and ran the relevant tests locally.