Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Record authors and maintainers for each architecture #145

Merged
merged 2 commits into from
Mar 15, 2024
Merged

Conversation

Luthaf
Copy link
Contributor

@Luthaf Luthaf commented Mar 13, 2024

This way we'll know who to ask for review when someone tries to modify some architecture, and we can include the authors list in the metadata.

I went the the following assignment, but it's only what I saw! If I missed any contributions please let me know and I'll add it to the files!

SOAP BPNN: @frostedoyster is the only author and maintainer
Alchemical: @abmazitov is the only author and maintainer
PET: @serfg, @frostedoyster and @abmazitov are authors (is this right?) and @serfg and @abmazitov are maintainers.

TODO:

  • read this data and show it in the docs

📚 Documentation preview 📚: https://metatensor-models--145.org.readthedocs.build/en/145/

@frostedoyster
Copy link
Collaborator

The tests are also failing on main, we're looking into it.

Are we sure that we need authors? How is this useful for the user?

Regarding the maintainers, perhaps it would be a good idea to only have one maintainer per architecture. If the maintainer needs help, of course they can ask other people. But in principle the user only wants one e-mail address, and having more maintainers IMO makes it too easy for each maintainer to think the model is someone else's responsibility

@Luthaf
Copy link
Contributor Author

Luthaf commented Mar 13, 2024

Are we sure that we need authors? How is this useful for the user?

Not extremely useful to the end user, but nice for the authors to get recognition.

I don't mind too much for maintainers, I agree that we should not have 50 different maintainers, but up to 3 seems reasonable to me. We can discuss this in tomorrow's meeting.

@frostedoyster
Copy link
Collaborator

frostedoyster commented Mar 13, 2024

Also, the maintainer should at least review the PRs to their model (#131...), otherwise how can they maintain code that they've never seen?

@Luthaf
Copy link
Contributor Author

Luthaf commented Mar 13, 2024

Yes, that's what the CODEOWNERS file should do. I'm not sure if it will just ask for review but allow to merge without one from that person, or require a review from that person for merging. There might be a setting for it.

Copy link
Collaborator

@frostedoyster frostedoyster left a comment

Choose a reason for hiding this comment

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

Works for me

@Luthaf Luthaf merged commit 8d59de4 into main Mar 15, 2024
11 checks passed
@Luthaf Luthaf deleted the model-maintainers branch March 15, 2024 14:28
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.

None yet

2 participants