-
Notifications
You must be signed in to change notification settings - Fork 33
Remove hardcoded vendor specific information #1760
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
23ebb10 to
6a46ebf
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1760 +/- ##
==========================================
+ Coverage 75.03% 75.07% +0.04%
==========================================
Files 84 84
Lines 11889 11910 +21
==========================================
+ Hits 8921 8942 +21
Misses 2968 2968
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:
|
6a46ebf to
1536238
Compare
|
@yarikoptic Is Lines 131 to 139 in 7cf6ace
identifier of the Dandiset model ever be a dictionary?
|
running tests did not step that way and originally added in 9006f96 which lead to dandi/dandi-archive#63 (comment) which shows that indeed it something which was changed on dandi-archive side awhile back. Thus we can safely remove this block IMHO. |
So that the checks of `id` and `identifier` in metadata allow different instance names
So that the check of `id` in metadata allows different instance names
1536238 to
c28e19d
Compare
|
should it be taken out of the draft? |
It is a base PR that I will merge other PRs into. I will seek approval from you to merge other PRs into this one. Each of the other PRs warrant a judgment. An example of this those PRs is #1767. |
To reflect that instance names are no longer restricted to "DANDI"
|
I think we can leave the "DANDI:" string in the following lines in place. It is specific to how datasets in the DANDI Archive are published on identifiers.org. dandi-cli/dandi/dandiarchive.py Lines 608 to 617 in f5b8bc3
@yarikoptic Let me know if I am wrong. |
…from_doi` This is only a replacement for the code that strips the "DANDI:" prefix. The replacement works for dandisets of different DANDI instances. However, `update_dandiset_from_doi` is far from robust. This replacement doesn't fix underlying weakness stems from assumptions. Further improvements of `update_dandiset_from_doi` are needed.
|
I think that keys such as For example, Lines 317 to 324 in f5b8bc3
@yarikoptic Let me know if I am wrong. |
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 removes hardcoded vendor-specific information (particularly "DANDI:" prefix references) from the codebase and replaces them with vendor-agnostic implementations using configuration from dandischema. The changes allow the CLI to work with different DANDI instances without hardcoded dependencies on the DANDI Archive instance.
Key changes:
- Replaced hardcoded "DANDI:" string checks with dynamic pattern matching using
ID_PATTERNfrom dandischema - Removed vendor-specific validation logic from identifier parsing
- Updated prefix stripping to handle any known instance name instead of just "DANDI:"
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| dandi/tests/test_download.py | Updated assertion to use pattern matching instead of hardcoded "DANDI:" prefix |
| dandi/dandiset.py | Removed hardcoded validation for "DANDI" propertyID in identifier records |
| dandi/dandiarchive.py | Updated comment to be vendor-agnostic |
| dandi/cli/tests/test_service_scripts.py | Changed assertions to use pattern matching with ID_PATTERN |
| dandi/cli/cmd_service_scripts.py | Generalized prefix stripping to handle any known instance name |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
* feat: reimplement `dandi.utils.is_url` So that it can determine whether a given string is a standard HTTP, HTTPS, FTP URL, or DANDI URL more precisely. Additionally, this solution supports DANDI URL of different DANDI archive instances. * test: Add tests for `dandi.utils.is_url`
|
Yes, both identifiers and checksum are ok to remain dandi. Note that some test run is failing... Otherwise this is ready? |
Yes, this is ready. The failure in test is unrelated this PR since it appears in #1768 as well. I am looking into the cause of the failure now. |
Configuer CI to test against a vendor specific DANDI API other than the default instance of DANDI API
|
🚀 PR was released in |
This PR closes #1752. It removes any hardcoded vendor specific information that are now made available through a
Configobject defined in dandischema. Additionally, it includes the changes in GH workflow in #1771 to verify the correctness of the removal of the vendor specific information.Specifically, this PR replaces any use of the
"DANDI:"string that is associated with the DANDI Archive instance with vendor agnostic (or DANDI instance agnostic) setup.TODOs:
dandi.utils.is_url#1767 into this PR.Notes:
dandi service-scripts publish-dandiset-version-doiis not replaced/removed in this PR. It will be part of Make thedandi service-scripts publish-dandiset-version-doicommand DANDI instance specific, incorporating vendor information of a particular DANDI instance #1704 instead.obolibrary#1769.Release Notes
Removed hardcoded vendor specific info so that dandi-cli can now connect to different DANDI instances with different vendor specific info.