Subproject ngff-spec under specfications directory#404
Conversation
- update build tools to handle ngff-spec contents - Make sure schemas are provided as html artefacts - deprecate bikeshed
Automated Review URLs |
|
should we move with
preview on your link looks good, though |
|
@lubianat #408 makes total sense to go in first. On second and third thought, I think I may also undo the RTD deprecation in here. My original reason for doing this in the first place was the lack of customization options on how to integrate ngff-spec pages in the ngff page. But since the placing of the spec submodules under the |
- specifications/0.5: Updated to f3729f5 (untrack built bs files) - specifications/0.6.dev2: Updated to da4606b (0.6.dev2 tag) - specifications/Dev: Updated to 05b8089 (main branch)
|
@lubianat not at all :) We only merged the backport for 0.1...0.4 last week and I haven't updated the submodule configuration here yet. Will do asap! |
|
@lubianat thanks for the review
|
|
I see! On the ngff-spec repo, the footer exists on the built pages: Where do you think would be the correct place to put it? Would it collide with the copyright statement that's already on the page? |
|
Oh, I had not seen that. I think this may be more "political" than I can decide on, but perhaps the NGFF credit is enough? Maybe that is a @joshmoore question, maybe for the PR review... |
lubianat
left a comment
There was a problem hiding this comment.
I think it is in a safe shape to merge. Not perfect, perhaps, but good enough.
|
I think this is ready to pull the trigger so we can go forward with other things :) |
|
This pull request has been mentioned on Image.sc Forum. There might be relevant details there: https://forum.image.sc/t/ngff-weekly-dev-update-thread/110810/56 |
|
Super excited to see this go in. 🎉 Thanks to @jo-mueller for driving the refactoring and to everyone for the review. There will likely be a series of smaller changes from here on out. Please feel free to open issues if you see anything that needs fixing. |
There was a problem hiding this comment.
Pull request overview
This PR reorganizes the NGFF specifications by moving them from root-level submodules to a dedicated specifications directory, while transitioning from Bikeshed to Markdown/Jupyter-Book format. The changes enable unified documentation hosting at ngff.openmicroscopy.org with proper redirects from legacy URLs.
Changes:
- Relocated version submodules (0.1-0.5, dev, latest) from repository root to
specifications/directory, pointing to the new ome/ngff-spec repository - Replaced Bikeshed-based build process with Jupyter-Book/MyST workflow for specification rendering
- Added bibliography support and redirects to maintain backward compatibility with existing specification URLs
Reviewed changes
Copilot reviewed 21 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| specifications/index.md | Updated version links to use Markdown references and toctree structure instead of HTML |
| specifications/{dev,0.5,0.4,0.3,0.2,0.1} | New submodule references pointing to ome/ngff-spec branches |
| requirements.txt | Added sphinxcontrib-bibtex dependency, removed duplicate sphinx-reredirects |
| references.bib | Added bibliography entries for NGFF, Zarr, N5, and ome-zarr-py |
| latest, 0.5, 0.4, 0.3, 0.2, 0.1 | Removed old root-level submodules |
| conf.py | Replaced Bikeshed processing with build_served_html() function for specifications |
| .readthedocs.yaml | Added Node.js 22 toolchain and pip installation for specifications/dev |
| .gitmodules | Updated all submodule paths to specifications/ directory with ome/ngff-spec URLs |
| .gitignore | Renamed _bikeshed directory to _html_extra |
| .github/workflows/validation.yml | Commented out validation workflow |
| .github/workflows/review.yml | Commented out review URL workflow |
| .github/workflows/dependabot.yml | Added Dependabot configuration for specifications/dev submodule |
| .codespellrc | Excluded references.bib from spell checking |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <li> <a href="../0.2/index.html">0.2</a> March 2021</li> | ||
| <li> <a href="../0.1/index.html">0.1</a> November 2020</li> | ||
| </ul> | ||
| The current released version of the OME-Zarr specification is [0.5](0.5/ngff_spec/index.md). |
There was a problem hiding this comment.
The link path appears incorrect. Line 6 references 0.5/ngff_spec/index.md but line 13 in the toctree references 0.5/index.md. These paths should be consistent, and based on the toctree structure, line 6 should likely point to 0.5/index.md instead.
| The current released version of the OME-Zarr specification is [0.5](0.5/ngff_spec/index.md). | |
| The current released version of the OME-Zarr specification is [0.5](0.5/index.md). |
There was a problem hiding this comment.
@jo-mueller - I think copilot spotted a true bug here. The link there is leading to nothing indeed:
see https://ngff.openmicroscopy.org/specifications/index.html#0.5/ngff_spec/index.md
maybe we can add this to #440?
| subprocess.check_call( | ||
| f"bikeshed spec {index_file} {output_file}", shell=True, | ||
| ) | ||
| subprocess.check_call([sys.executable, script]) |
There was a problem hiding this comment.
The subprocess call lacks error handling or informative error messages. If the pre_build.py script fails, users won't know which version or script caused the failure. Consider wrapping in a try-except block with a descriptive error message indicating the version and script path.
| subprocess.check_call([sys.executable, script]) | |
| try: | |
| subprocess.check_call([sys.executable, script]) | |
| except subprocess.CalledProcessError as e: | |
| print(f'❌ Failed to run pre_build script for version {version} at {script}: return code {e.returncode}') | |
| raise | |
| except Exception as e: | |
| print(f'❌ Unexpected error while running pre_build script for version {version} at {script}: {e}') | |
| raise |
There was a problem hiding this comment.
This is the kind of thing I see as extra copilot noise. A decision with trade-offs that it is making. Could be better in theory, but in real life adds burdens around
| shutil.rmtree(target_dir) | ||
| shutil.copytree(output_dir, target_dir) | ||
| # build jupyter-book docs in specification submodules | ||
| myst_file = glob.glob(f'specifications/{version}/**/myst.yml', recursive=True)[0] |
There was a problem hiding this comment.
Lines 123-124 define variables that are never used in the function. The myst_file variable is assigned but not referenced anywhere, and bikeshed_output is only used in the try-except block below. If myst_file serves no purpose, it should be removed to avoid confusion.
| myst_file = glob.glob(f'specifications/{version}/**/myst.yml', recursive=True)[0] |
There was a problem hiding this comment.
ok clean up. not necessary, not harmful.
clean codewise — only the slightly painful cognitive burden of reviewing a bot suggestion (which is not nothing)


Fixes #399
This is a proposed solution on how a joint layout of the ngff page and the future home of the spec, ome/ngff-spec could look like.
Key changes:
specificationsfolder here in ngff/0.5/index.html --> /specifications/0.5/index.html
/0.5 --> /specifications/0.5
https://ngff.openmicroscopy.org/0.5/schemas/image.schemaTODO
specificationssection?