-
Notifications
You must be signed in to change notification settings - Fork 33
BF: strip away any prefix before ":" while determining numeric dandiset identifier #1751
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1751 +/- ##
==========================================
- Coverage 75.05% 75.05% -0.01%
==========================================
Files 84 84
Lines 11874 11873 -1
==========================================
- Hits 8912 8911 -1
Misses 2962 2962
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Pull request overview
This PR fixes identifier extraction to handle prefixed dandiset identifiers (e.g., "DANDI-SANDBOX:217838") by generalizing the prefix-stripping logic to remove any prefix before the last colon, rather than only stripping the "DANDI:" prefix.
Key Changes:
- Replaced
startswith("DANDI:")check withsplit(":")[-1]to extract the numeric identifier from any prefixed format - Updated comments to reflect the uncertainty about the original PR #348 and mark the code for future refactoring
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@yarikoptic To get the CI tests to pass, we need to first merge #1744. |
…et identifier We have it confusing, as the identifier in the client model is just the numeric part. We should have called it differently from the metadata model identifier
faebfa1 to
96f056c
Compare
|
merged, and marked mystery resolved as IIRC we figured it out on Friday as it was not the identifier within dandiset.yaml as was retrieved from non-vendored back then instance. |
candleindark
left a comment
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.
Add a small change. Otherwise, it is approved.
Co-authored-by: Isaac To <candleindark@users.noreply.github.com>
|
@candleindark you would need to reapprove for it to become mergeable... feel welcome to click merge as well, not yet sure if worth releasing right away, may be more would come up? |
|
🚀 PR was released in |
We have it confusing, as the identifier in the client model is just the numeric part. We should have called it differently from the metadata model identifier.
That original code which added those divergences though seemed tried to make meditor possible:
Without this fix, we get confusing
which accents such divergence of definition of "identifier". If we just remove stripping away the prefix, then upload stalls while retrying and getting 500s from the server (nothing in dandi-cli logs was useful to point to the issue).
TODOs: