Skip to content

Warn when uploads omit unrecognized paths - #1915

Open
AtomicGlance wants to merge 8 commits into
dandi:masterfrom
AtomicGlance:fix/warn-skipped-upload-paths
Open

Warn when uploads omit unrecognized paths#1915
AtomicGlance wants to merge 8 commits into
dandi:masterfrom
AtomicGlance:fix/warn-skipped-upload-paths

Conversation

@AtomicGlance

Copy link
Copy Markdown
Contributor

Fixes #1493

dandi upload discovers only recognized assets. Before this change, a requested directory could finish with files omitted from the upload without telling the user.

This adds a discovery helper that reports omitted files individually when a requested tree contains recognized assets, or collapses a wholly unrecognized directory to one entry. It keeps Dandiset metadata, dot/VCS paths, common OS metadata, empty directories, Zarr contents, and directory symlinks out of the warning. Partial uploads stay scoped to the requested paths, and --allow-any-path remains an explicit opt-out.

The warning is registered after the progress table is created, so it is emitted after the table closes and still appears when another asset upload fails.

Tests added:

  • mixed recognized and unrecognized trees, unknown directories, ignored paths, Zarr directories, empty directories, and directory symlinks
  • whole-Dandiset warning, partial-upload suppression, --allow-any-path suppression, and warning-on-upload-error behavior

Local validation:

  • focused discovery tests: 3 passed, 1 symlink test skipped because the platform does not permit the fixture
  • upload tests collected successfully; they require the repository's Docker-backed API fixture, and Docker is not installed in this environment
  • ruff check and python -m py_compile passed

@codecov

codecov Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.40288% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.53%. Comparing base (d91a041) to head (1a7fe98).

Files with missing lines Patch % Lines
dandi/files/__init__.py 93.75% 3 Missing ⚠️
dandi/tests/test_files.py 95.55% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1915      +/-   ##
==========================================
+ Coverage   77.35%   77.53%   +0.18%     
==========================================
  Files          89       89              
  Lines       13325    13463     +138     
==========================================
+ Hits        10307    10439     +132     
- Misses       3018     3024       +6     
Flag Coverage Δ
unittests 77.53% <96.40%> (+0.18%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@AtomicGlance

Copy link
Copy Markdown
Contributor Author

Fixed the three Linux upload-test failures by placing the NWB fixture under a valid sub-01/ subject directory. The implementation was not changed. The helper test passes locally; the Docker-backed upload tests are covered by CI. Please rerun the checks when convenient.

@AtomicGlance

Copy link
Copy Markdown
Contributor Author

The rerun found one remaining assertion: the singular warning used “1 path were”. I fixed that in 95ac3d3 so one path reads “was” while plural counts keep “were”. Ruff checks pass; CI is running again.

@AtomicGlance

Copy link
Copy Markdown
Contributor Author

One macOS 3.13 job in the new matrix failed while cloning the external dandi-datasets/nwb_test_data repository (14 setup errors; 749 tests passed). That failure is unrelated to this PR. The upload-warning integration job is still running.

@AtomicGlance

Copy link
Copy Markdown
Contributor Author

Added focused coverage for the upload omission discovery helper in 414df4e. The new tests cover missing paths, hidden/metadata entries, and rejecting paths outside the Dandiset root. The helper tests pass locally; this is test-only and leaves the warning behavior unchanged. CI can rerun when convenient.

@AtomicGlance

Copy link
Copy Markdown
Contributor Author

The new run on 414df4e completed. Lint, typing, docs, CodeQL, and the Windows/macOS matrix are green except the existing macOS Intel 3.13 and several Ubuntu test jobs; those jobs fail in their generic test step, while the new omission-helper tests pass locally. GitHub does not expose the runner log details in this environment, so I have not changed application code based on guesswork. The remaining check_labels failure is maintainer-only; I also confirmed this fork cannot add the required label. Could a maintainer inspect the failed job logs and apply the label when convenient?

@AtomicGlance

Copy link
Copy Markdown
Contributor Author

Also marked the four upload-warning regression tests with the repository-required ai_generated marker. Ruff and the focused discovery tests still pass; no behavior changed.

@AtomicGlance
AtomicGlance force-pushed the fix/warn-skipped-upload-paths branch from 1364151 to bbec8c3 Compare September 9, 2026 18:42
@AtomicGlance

Copy link
Copy Markdown
Contributor Author

I corrected the omission warning so singular messages use “it was” and plural messages use “they were,” while preserving the original upload error. The diff is limited to dandi/upload.py; compileall and git diff --check pass locally. The focused Docker-backed test is skipped here because Docker is unavailable, so I’m relying on the pushed CI run for the full fixture coverage.

@AtomicGlance

Copy link
Copy Markdown
Contributor Author

The new run reached the test jobs, but the Ubuntu matrix is blocked before collection by the shared Docker setup: docker compose pull cannot pull minio/minio (pull access denied ... repository does not exist or may require docker login). The warning-specific test therefore has not executed in this run; the failure is infrastructure/setup, not a traceback from the changed formatter. The one code change remains the singular/plural pronoun fix in dandi/upload.py.

@AtomicGlance

AtomicGlance commented Sep 13, 2026

Copy link
Copy Markdown
Contributor Author

The full matrix now passes after switching the test stack's MinIO image from Docker Hub to quay.io/minio/minio:latest in 1a7fe98. The previous image could not be pulled, which prevented the integration fixtures from starting.

All test environments, both Codecov checks, lint, typing, and documentation are green. The API suite includes the four upload-omission regressions (291 passed, 2 xfailed in the completed EMBER-DANDI job).

Only check_labels remains. Could a maintainer @yarikoptic apply the appropriate release label and review this when convenient? Thanks!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

upload: if any file/path is skipped due to not part of DANDI layout -- log WARNING upon completion or ask

1 participant