Skip to content

Fix Windows-incompatible mkdir and add COCO auto-download to tutorial notebook - #613

Open
ashishsoni-ai wants to merge 3 commits into
JdeRobot:masterfrom
ashishsoni-ai:fix/tutorial-windows-coco-setup
Open

ashishsoni-ai wants to merge 3 commits into
JdeRobot:masterfrom
ashishsoni-ai:fix/tutorial-windows-coco-setup

Conversation

@ashishsoni-ai

Copy link
Copy Markdown

What

Fixes tutorial_image_detection.ipynb so it runs end-to-end on Windows without manual intervention.

Why

  • !mkdir -p fails on Windows (cmd.exe doesn't support the -p flag), throwing a SyntaxError
  • The COCO dataset cell had placeholder paths (<PATH_TO_COCO_VAL2017_IMAGES>) with no download guidance, blocking new contributors from running the notebook at all

Changes

  • Replaced the Unix-only !mkdir -p calls with cross-platform os.makedirs(..., exist_ok=True)
  • Added a cell that automatically downloads and extracts COCO annotations, with an option to either:
    • use a small subset (default, ~20 images, fast) for quick testing, or
    • download the full val2017 dataset for complete evaluation
  • Updated the dataset-loading cell to use the downloaded paths instead of placeholders

Testing

Ran the full notebook end-to-end on Windows 11 (Python 3.11, PyTorch 2.4.1 CPU) — all cells execute successfully, including model inference, evaluation metrics, and the precision-recall curve output.

Fixes #612

@dpascualhe

Copy link
Copy Markdown
Collaborator

Hi @ashishsoni-ai , nice contribution! The only thing that needs fixing before merging is removing duplicated imports and moving those that are actually new to the first cell for consistency

… notebook

- Replace Unix-only '!mkdir -p' with cross-platform os.makedirs()
- Add automated COCO val2017 download/extraction with small-subset
  option so the notebook runs end-to-end without manual dataset setup
  or large downloads

Fixes JdeRobot#612
@ashishsoni-ai
ashishsoni-ai force-pushed the fix/tutorial-windows-coco-setup branch from 9b920e6 to b8e94ff Compare July 10, 2026 15:52
@ashishsoni-ai

Copy link
Copy Markdown
Author

Thanks for the review @dpascualhe . Cleaned up the duplicate imports and moved the new ones into the first cell. Let me know if anything else needs adjusting — happy to help with more issues if there are any you'd point me toward.

@ashishsoni-ai

Copy link
Copy Markdown
Author

Hi @dpascualhe, just following up on this PR. I addressed the requested changes by removing the duplicate imports and moving the new imports into the first cell for consistency.

I also noticed the PR now has merge conflicts with the base branch. If you'd prefer, I can rebase and resolve the conflicts as well.

I'd appreciate another review whenever you have time. Thanks!

@dpascualhe

Copy link
Copy Markdown
Collaborator

Hi @ashishsoni-ai ! Thanks for addressing the changes, the new downloading logic is working perfectly fine. As you said, there are some conflicts now, can you resolve those? Besides, the cell that Run evaluation on a subset of the dataset can be simplified now, as you are already defining the size of the subset in the first cell and could use directly the dataset object instead of defining a new small_dataset. After that, we are ready to merge 😄

…ows-coco-setup

Only conflict was examples/tutorial_image_detection.ipynb metadata: kept upstream/master's kernelspec (python3 / Python 3 (ipykernel)) and language_info version, dropping the branch's local 'perceptionmetrics' kernel metadata. All other notebook changes merged cleanly.
Run evaluation directly on the dataset object instead of building and swapping in a small_dataset slice: the dataset size is already controlled by USE_SMALL_SUBSET / SUBSET_SIZE in the download cell, so the temporary swap and its restore logic are no longer needed (addresses review feedback on JdeRobot#613).
@ashishsoni-ai

Copy link
Copy Markdown
Author

Hi @dpascualhe, both points are addressed and pushed:

  • Conflicts: merged master into the branch — the only conflict was the notebook metadata (kernelspec / language_info), where I kept upstream's values. Interestingly, upstream had already landed the same os.makedirs fix, so that part merged cleanly.
  • Evaluation cell: simplified as suggested — it now runs detection_model.eval(dataset=dataset, ...) directly, since the dataset is already limited to SUBSET_SIZE in the first cell. The small_dataset slice, the temporary dataset.dataset swap and its finally restore are gone.

The PR is conflict-free now. I'll refresh the stored notebook outputs (they still show the old 5-image evaluation) in a moment — after that we should be ready to merge. Thanks for the review!

This branch has not been deployed

No deployments
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.

tutorial_image_detection.ipynb fails on Windows: Unix-only shell command and missing COCO dataset setup instructions

3 participants