Skip to content

Architecture coverage test#803

Draft
sofiia-chorna wants to merge 41 commits intomainfrom
architecture-coverage-test
Draft

Architecture coverage test#803
sofiia-chorna wants to merge 41 commits intomainfrom
architecture-coverage-test

Conversation

@sofiia-chorna
Copy link
Collaborator

@sofiia-chorna sofiia-chorna commented Oct 8, 2025

  • collect coverage for the architectures, make them visible in codecov.io
  • for threshold, use only utils & cli
  • visual badge for coverage of the architectures in the doc

Please go the the Preview tab and select the appropriate PR template:


📚 Documentation preview 📚: https://metatrain--803.org.readthedocs.build/en/803/

@sofiia-chorna
Copy link
Collaborator Author

with:
python-version: "3.13"
- run: pip install tox
- run: pip install tox coverage[toml]
Copy link
Contributor

@pfebrer pfebrer Oct 8, 2025

Choose a reason for hiding this comment

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

Is it possible to do this in tox.ini instead of here in the workflow file?

Copy link
Contributor

Choose a reason for hiding this comment

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

Then it would generate the coverage also when running locally

Copy link
Collaborator Author

@sofiia-chorna sofiia-chorna Oct 8, 2025

Choose a reason for hiding this comment

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

Hmm worth trying then, thanks for suggestion

@Luthaf
Copy link
Member

Luthaf commented Oct 9, 2025

seems to work [...]

The llpr/gap/nanopet parts of the code should also have tests, it is weird there is 0 coverage reported for these.


.. image:: https://codecov.io/gh/metatensor/metatrain/branch/main/graph/badge.svg?flag=coverage_nanopet
:target: https://codecov.io/gh/metatensor/metatrain/tree/main/src/metatrain/deprecated/nanopet

Copy link
Contributor

Choose a reason for hiding this comment

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

It's very nice to show this here. Right now they show as codecov: unknown, they will work once the PR is merged to main?

It would be better to add the badge automatically instead of manually in each documentation.py file. The string could be added here:

docstring = (
f".. _architecture-{template_variables['architecture']}:" + "\n\n" + docstring
)

after f".. _architecture-{template_variables['architecture']}:" + "\n\n". You can try to do it and let me know if you have doubts :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it is unknown as it is not in main. here is the example of badge pointing to this branch:
https://codecov.io/gh/metatensor/metatrain/branch/architecture-coverage-test/graph/badge.svg?flag=coverage_nanopet

I wouldn't do it atomatically as it should be also set up in .codecov.yml

Let me know if you see better approach please

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I see, but if we do it automatically and it is not set in .codecov.yml it will just show "Unknown" no? Seems fine.

Also I would then add the instruction to add your architecture in .codecov.yml here: https://github.com/metatensor/metatrain/blob/main/docs/src/dev-docs/new-architecture.rst

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah okay, seems fine indeed. Thanks

@Luthaf
Copy link
Member

Luthaf commented Dec 5, 2025

So I'm not 100% sure the way we partition the coverage between architectures is working, it looks like we would have to merge first to know about this.

Looking around codecov, this seems very relevant: https://docs.codecov.com/docs/components, should we use it instead of multiple projects?

@sofiia-chorna
Copy link
Collaborator Author

So I'm not 100% sure the way we partition the coverage between architectures is working, it looks like we would have to merge first to know about this.

Indeed, very good point. Checking the components

@sofiia-chorna
Copy link
Collaborator Author

Converting to draft to play with it (sorry, forking does not work with codecov)

@sofiia-chorna sofiia-chorna marked this pull request as draft December 5, 2025 18:22
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