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

Add some kind of --path-type [exact|glob] for delete #1446

Open
yarikoptic opened this issue May 23, 2024 · 10 comments
Open

Add some kind of --path-type [exact|glob] for delete #1446

yarikoptic opened this issue May 23, 2024 · 10 comments
Assignees

Comments

@yarikoptic
Copy link
Member

yarikoptic commented May 23, 2024

ATM we have that option for download command.

I am yet not sure how CLI should look like given that we take target file paths or URIs. I guess we could add this option and then treat local paths as globs to be expanded by python locally and on the server. For URLs we would need some custom URI schema to indicate that it is a glob for a particular dandiset (e.g. dandi-glob://<instance name>/<dandiset id>[@<version>][/<glob>])

WDYT @jwodder ?

Prompted by

although not yet known if really needed in that case.

@yarikoptic
Copy link
Member Author

ah, actually in https://dandi.readthedocs.io/en/latest/ref/urls.html#resource-ids we already specify that if there is such an option and =glob then we have dandi://<instance name>/<dandiset id>[@<version>][/<glob>] so I guess the path is "clear" right?

@jwodder
Copy link
Member

jwodder commented May 23, 2024

@yarikoptic

I guess we could add this option and then treat local paths as globs to be expanded by python locally and on the server.

A local glob path should be expanded by the shell into the full list of paths, at which point dandi delete shouldn't need to do anything special.

@yarikoptic
Copy link
Member Author

I guess we could add this option and then treat local paths as globs to be expanded by python locally and on the server.

A local glob path should be expanded by the shell into the full list of paths, at which point dandi delete shouldn't need to do anything special.

in case of local path specification -- do we remove only "matching" filenames or if e.g. I specify to remove a directory, would I remove all assets (which might be more or less than I have locally) from that "directory" on the server? If it is just 1-to-1 match to local files in the folder, then indeed we could keep it consistent and pretty much ignore that option for local paths. If not -- then it is a matter of passing glob not expanded (quote it) and match in code locally and on server.

@jwodder
Copy link
Member

jwodder commented May 23, 2024

@yarikoptic If a local directory path is passed to dandi delete, it deletes from the server each asset whose path starts with the given directory path; the local directory contents are ignored.

@yarikoptic
Copy link
Member Author

ok, in case of --path-type=glob let's use provided URL as a glob (per comment above) and if not a URL (ie local path) - using glob.glob(PATH, recursive=True) locally and delete matching assets (or folders if glob ended with /) locally and remotely.

@jwodder
Copy link
Member

jwodder commented May 31, 2024

if not a URL (ie local path) - using glob.glob(PATH, recursive=True) locally

Why wouldn't we just let globs be expanded by the user's shell?

and delete matching assets (or folders if glob ended with /) locally and remotely.

dandi delete currently doesn't delete anything on the local side. Doing so would be a considerable change that, if desired, should be submitted as a separate issue.

@yarikoptic
Copy link
Member Author

if not a URL (ie local path) - using glob.glob(PATH, recursive=True) locally

Why wouldn't we just let globs be expanded by the user's shell?

for that you do not need any --path-type or in other words it is what you get if use --path-type exact. Stating --path-type glob would mean that user is specifying a glob explicitly (and better quotes so shell does not expand), and possibly benefits from local shell not having recursive glob matching at all. I think it would be nice to just be able to say delete --path-type glob '*_entity-bad*' and have all assets with _entity-bad be removed locally/remotely.

@yarikoptic
Copy link
Member Author

and delete matching assets (or folders if glob ended with /) locally and remotely.

dandi delete currently doesn't delete anything on the local side. Doing so would be a considerable change that, if desired, should be submitted as a separate issue.

frankly I didn't realize that! then let's forget about "local" aspects in my above phrases. Should also make implementation easier, and we would file an issue if some user desires it is worth it ;-)

@jwodder
Copy link
Member

jwodder commented Jun 3, 2024

@yarikoptic

if not a URL (ie local path) - using glob.glob(PATH, recursive=True) locally

As I stated above, local (non-URL) paths passed to dandi delete are only matched against server paths; the only role of the local Dandiset is in ensuring the paths are relative to the Dandiset root. If we want dandi delete to handle globbing of local/file/non-URL paths, shouldn't the globbing happen against server paths rather than local paths for consistency?

Also, do we want to support local path arguments that contain a glob in the Dandiset path portion, e.g., dandi delete /home/user/00000*/*.nwb when run inside a Dandiset at /home/user/000001? That would be tricky.

@yarikoptic
Copy link
Member Author

If we want dandi delete to handle globbing of local/file/non-URL paths, shouldn't the globbing happen against server paths rather than local paths for consistency?

yes, globbing should happen against server paths, likely just by passing the glob=True to assets listing endpoint.

Also, do we want to support local path arguments that contain a glob in the Dandiset path portion, e.g., dandi delete /home/user/00000*/*.nwb when run inside a Dandiset at /home/user/000001? That would be tricky.

Then let's do no such thing -- just state that in case of =glob passed to server as is and assumed from the top of the dandiset.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants