Skip to content

Add preparation step for Jekyll deployment and include static files#4631

Merged
shyuep merged 3 commits intomaterialsproject:masterfrom
cogsworth37:patch-4626-include-styles
Apr 11, 2026
Merged

Add preparation step for Jekyll deployment and include static files#4631
shyuep merged 3 commits intomaterialsproject:masterfrom
cogsworth37:patch-4626-include-styles

Conversation

@cogsworth37
Copy link
Copy Markdown
Contributor

Summary

Major changes:

Fixes broken styles/JS introduced by #4626 — the CI migration didn't account for Sphinx asset handling.

Checklist

  • Google format doc strings added. Check with ruff.
  • Type annotations included. Check with mypy.
  • Tests added for new features/fixes.
  • If applicable, new classes/functions/modules have duecredit @due.dcite decorators to reference relevant papers by DOI (example)

Tip: Install pre-commit hooks to auto-check types and linting before every commit:

pip install -U pre-commit
pre-commit install

rm -rf .apidoc_merge
rm apidoc/*.rst
mv html/pymatgen*.html .
mv html/modules.html .
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great! thanks for fixing this. I have no experience with sphinx/jekyll, but I think the copy step could just go here for simplicity?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I did it this way to create a clear boundary. the cp -r command must be run after the sphinx-build command. while I could put it at the end of the block, I wanted to create no confusion if the order gets messed up.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think both HTML and static are built by sphinx, looks quite confusing to me to copy HTML first and then in another step copy static?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That is a fair point. would you prefer a collapse into that block or all rm/mv/cp commands be moved to a more operational block?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe let's combine them? making this workflow shorter would make later reading/debug a bit easier (personally)...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's moved

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thanks

Copy link
Copy Markdown
Contributor

@DanielYang59 DanielYang59 Apr 10, 2026

Choose a reason for hiding this comment

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

pymatgen doesn't have docs optional dependency

WARNING: pymatgen 2026.3.23 does not provide the extra 'docs'

it's currently declared via uv dependency group docs

these two steps (pip install pymatgen and then sphinx) could just be uv sync --group docs

Copy link
Copy Markdown
Contributor

@DanielYang59 DanielYang59 Apr 10, 2026

Choose a reason for hiding this comment

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

can you also help me remove docs from default dependency group (and that TODO)? I believe it's not needed anymore

pymatgen/pyproject.toml

Lines 283 to 285 in f475a89

[tool.uv]
# TODO: remove `docs` once doc generation is automated
default-groups = ["dev", "test", "lint", "docs"]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated to use uv sync --group docs and removed docs from the default groups

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

great thanks

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

these two sphinx commands should be called via uv instead, i.e. uv run sphinx-xxx ...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated to use uv commands here

working-directory: docs
run: |
cp -r html/_static _static
touch .nojekyll
Copy link
Copy Markdown
Contributor

@DanielYang59 DanielYang59 Apr 10, 2026

Choose a reason for hiding this comment

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

Curious what does .nojekyll do (as i don't have experience with jekyll at all)? I see here:

It is now possible to completely bypass Jekyll processing on GitHub Pages by creating a file named .nojekyll in the root of your pages repo and pushing it to GitHub.

But I think in the _config.yml there is already an explicit include: _static tag? does these two serve different purposes?

This should only be necessary if your site uses files or directories that start with underscores since Jekyll considers these to be special resources and does not copy them to the final site.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It does not. it's an extra extra guard. I usually like to add protections in multiple places as long as it's functionally consistent. just a quirk of mine, happy to remove if you don't feel it's necessary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess we're not talking about security where we need layers of defense here... personally I think if one config gets the job done then we might not need to duplicate here (less confusion for later readers... people like me would think they're serving different purposes)

@shyuep shyuep merged commit ccc16d2 into materialsproject:master Apr 11, 2026
6 checks passed
@DanielYang59
Copy link
Copy Markdown
Contributor

Great style seems to be working now!

image

@DanielYang59
Copy link
Copy Markdown
Contributor

I noticed the links to sources are not working (for both core and non-core modules), not sure if you would have time for a look? @cogsworth37

For example for non-core ext:

# Current URL generated in docs
- https://github.com/materialsproject/src/pymatgen/ext/optimade.py

# Correct URL
+ https://github.com/materialsproject/pymatgen/blob/master/src/pymatgen/ext/optimade.py
image

For core Ion it points to the wrong repo:

# Current URL generated in docs
- https://github.com/materialsproject/pymatgen/blob/v2026.4.7/src/pymatgen/core/ion.py

# Correct URL
+ https://github.com/materialsproject/pymatgen-core/blob/main/src/pymatgen/core/ion.py
image

@shyuep
Copy link
Copy Markdown
Member

shyuep commented Apr 11, 2026

The sources are in different locations. The core classes are in the pymatgen-core repo. Actually, we can also just use the submodule since it is already linked.

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.

3 participants