Skip to content

Make dandi upload look for dandiset.yaml in parents of common path #916

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.

Sign up for GitHub

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

Merged
yarikoptic merged 1 commit into from
Feb 17, 2022
Merged

Make dandi upload look for dandiset.yaml in parents of common path #916

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 3 additions & 8 deletions dandi/cli/cmd_upload.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,6 @@


@click.command()
# @dandiset_path_option(
# help="Top directory (local) of the dandiset. Files will be uploaded with "
# "paths relative to that directory. If not specified, current or a parent "
# "directory containing dandiset.yaml file will be assumed "
# )
@click.option(
"-e",
"--existing",
Expand Down Expand Up @@ -69,7 +64,6 @@ def upload(
dandi_instance,
existing="refresh",
validation="require",
dandiset_path=None,
# Development options should come as kwargs
allow_any_path=False,
upload_dandiset_metadata=False,
Expand All @@ -79,7 +73,9 @@ def upload(
Upload Dandiset files to DANDI Archive.

The target Dandiset to upload to must already be registered in the archive,
and a `dandiset.yaml` file must exist in the local `--dandiset-path`.
and a `dandiset.yaml` file must exist in the common ancestor of the given
paths (or the current directory, if no paths are specified) or a parent
directory thereof.

Local Dandiset should pass validation. For that, the assets should first
be organized using the `dandi organize` command.
Expand All @@ -100,7 +96,6 @@ def upload(
paths,
existing=existing,
validation=validation,
dandiset_path=dandiset_path,
dandi_instance=dandi_instance,
allow_any_path=allow_any_path,
upload_dandiset_metadata=upload_dandiset_metadata,
Expand Down
3 changes: 1 addition & 2 deletions dandi/tests/fixtures.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,8 +330,7 @@ def upload(
with pytest.MonkeyPatch().context() as m:
m.setenv("DANDI_API_KEY", self.api.api_key)
upload(
paths=paths,
dandiset_path=self.dspath,
paths=paths or [self.dspath],
dandi_instance=self.api.instance_id,
devel_debug=True,
**{**self.upload_kwargs, **kwargs},
Expand Down
19 changes: 8 additions & 11 deletions dandi/upload.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ def upload(
paths: Optional[List[Union[str, Path]]] = None,
existing: str = "refresh",
validation: str = "require",
dandiset_path: Union[str, Path, None] = None,
dandi_instance: str = "dandi",
allow_any_path: bool = False,
upload_dandiset_metadata: bool = False,
Expand All @@ -42,11 +41,15 @@ def upload(
from .dandiapi import DandiAPIClient
from .dandiset import APIDandiset, Dandiset

dandiset_ = Dandiset.find(dandiset_path)
if paths:
paths = [Path(p).absolute() for p in paths]
dandiset_ = Dandiset.find(os.path.commonpath(paths))
else:
dandiset_ = Dandiset.find(None)
if not dandiset_:
raise RuntimeError(
f"Found no {dandiset_metadata_file} anywhere. "
"Use 'dandi download' or 'organize' first"
f"Found no {dandiset_metadata_file} anywhere in common ancestor of"
" paths. Use 'dandi download' or 'organize' first."
)

instance = get_instance(dandi_instance)
Expand Down Expand Up @@ -74,15 +77,9 @@ def upload(

ignore_benign_pynwb_warnings() # so validate doesn't whine

#
# Treat paths
#
if not paths:
paths = [dandiset.path]
original_paths = paths

# Expand and validate all paths -- they should reside within dandiset
paths = [Path(p).absolute() for p in paths]
dandi_files = list(
find_dandi_files(
*paths,
Expand Down Expand Up @@ -335,7 +332,7 @@ def upload_agg(*ignored: Any) -> str:

if sync:
relpaths: List[str] = []
for p in original_paths:
for p in paths:
Copy link
Copy Markdown
Member

@yarikoptic yarikoptic Feb 17, 2022

Choose a reason for hiding this comment

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

I wonder if there was some good reason to use original_paths here instead of .absolute()ified ones... can't think of anything and (to be removed) comment on expanding and "validating" does not help (was there some which was removed? or did we use normpath not abspath at some point?)

Copy link
Copy Markdown
Contributor Author

@jwodder jwodder Feb 17, 2022

Choose a reason for hiding this comment

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

Prior to #853, the code around the assignment of original_paths looked like this:

dandi-cli/dandi/upload.py

Lines 71 to 74 in f3c62a3

original_paths = paths
# Expand and validate all paths -- they should reside within dandiset
paths = find_files(".*", paths) if allow_any_path else find_dandi_files(paths)

Copy link
Copy Markdown
Member

@yarikoptic yarikoptic Feb 17, 2022

Choose a reason for hiding this comment

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

I see, so they were overloaded with the ones from find*files which is no longer the case iirc, so not needed indeed. I don't think absolute should have negative effect there.. let's proceed. thanks!

rp = os.path.relpath(p, dandiset.path)
relpaths.append("" if rp == "." else rp)
path_prefix = reduce(os.path.commonprefix, relpaths) # type: ignore[arg-type]
Expand Down
4 changes: 3 additions & 1 deletion docs/source/cmdline/upload.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@
Upload Dandiset files to DANDI Archive.

The target Dandiset to upload to must already be registered in the archive, and
a :file:`dandiset.yaml` file must exist in the local :option:`--dandiset-path`.
a :file:`dandiset.yaml` file must exist in the common ancestor of the given
paths (or the current directory, if no paths are specified) or a parent
directory thereof.

Local Dandisets should pass validation. For that, the assets should first be
organized using the :ref:`dandi_organize` command.
Expand Down