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

🐕 Batch: Pre-submission lintern #903

Closed
miquelduranfrigola opened this issue Dec 7, 2023 · 10 comments
Closed

🐕 Batch: Pre-submission lintern #903

miquelduranfrigola opened this issue Dec 7, 2023 · 10 comments
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed medium Useful issue or future roadmap, and needs attention

Comments

@miquelduranfrigola
Copy link
Member

miquelduranfrigola commented Dec 7, 2023

Summary

Often, small errors like a short title in the metadata will cause the model incorporation workflows to fail. It would be good to identify these errors earlier using a lintern.

Ideally, we would have a command like:

ersilia lint --repo_path $PATH_TO_WIP_MODEL

Objectives

We can potentially check several things in a model repository, including:

  • The metadata.json file
  • The Dockerfile
  • The model/framework and model/checkpoints folders
  • The LICENSE

I would start with the metadata.json file. Most of the heavy lifting has already been done here: https://github.com/ersilia-os/ersilia/blob/master/ersilia/hub/content/card.py

A potential approach would be:

  1. Create a module called lintern.py under hub/content/
  2. Write a class called Lint with an arbitrary number of methods, including metadata.
  3. Write a CLI command that primarily uses the Lint class to check that everything is correct in the --repo_path folder.

Do the test as part of an earlier workflow. These are not mutually exclusive

Documentation

@miquelduranfrigola miquelduranfrigola added enhancement New feature or request help wanted Extra attention is needed labels Dec 7, 2023
@DhanshreeA DhanshreeA self-assigned this Feb 19, 2024
@DhanshreeA DhanshreeA added the medium Useful issue or future roadmap, and needs attention label Feb 19, 2024
@DhanshreeA DhanshreeA changed the title 📑 Pre-submission lintern [🐕 Batch]: Pre-submission lintern May 28, 2024
@DhanshreeA DhanshreeA moved this to Queue in Ersilia Model Hub May 28, 2024
@GemmaTuron
Copy link
Member

Is this lint check better suited for ersilia-maintenance? has any work in this direction been done or could this be an entry-level task @miquelduranfrigola and @DhanshreeA ?

@miquelduranfrigola
Copy link
Member Author

Given that we have plans to simplify the CLI commands, I would definitely not include a lint command. However, lint functionalities are included (and could be further expanded) in both the test command as well as the ModelInspector class (used in ersilia-maintenance).

We can certainly think of more "tests" or "inspections" to do and come up with a good entry-level task. This is a good idea.

@DhanshreeA
Copy link
Member

Plus to add to @miquelduranfrigola 's point, ersilia pack also has a lint built in for newer model contributions.

@GemmaTuron
Copy link
Member

Hi @DhanshreeA
Please if this is a good entry level task, open the appropriate issues in ersilia-maintenance repository or wherever appropriate.

@GemmaTuron GemmaTuron moved this to On Hold in Ersilia Model Hub Sep 24, 2024
@GemmaTuron GemmaTuron changed the title [🐕 Batch]: Pre-submission lintern 🐕 Batch: Pre-submission lintern Sep 24, 2024
@GemmaTuron
Copy link
Member

Hi @DhanshreeA

I am unsure whether this issue is tackled in independent issues or should remain open. Can you update? Thx

@DhanshreeA
Copy link
Member

DhanshreeA commented Oct 17, 2024

@GemmaTuron I think we already have a lot done on this front, namely, with the following tools/ersilia commands:

  1. ersilia inspect: More for maintenance purposes since this works "a posteriori", ie it checks models routinely that already exist in the hub.
  2. ersilia test: We want to enforce its usage during model contribution.
  3. Linting in ersilia-pack through ersilia_model_lint: This was designed with the intention of being used within the model contribution and submission workflow. However this only works with the new template.

We will have to consolidate functionality across the three of these, however it does not make sense to add another linter into Ersilia. So this issue is redundant at this point.

@GemmaTuron
Copy link
Member

Hi @miquelduranfrigola and @DhanshreeA

Should we close this model then?

@miquelduranfrigola
Copy link
Member Author

I'll let @DhanshreeA decide

@DhanshreeA
Copy link
Member

Yes, I think it's good to close this issue. We need to only focus on ersilia inspect, test, and the lint command within ersilia-pack (which can be added as a pipeline step on its own)

@miquelduranfrigola
Copy link
Member Author

This makes sense, @DhanshreeA - thanks

@DhanshreeA DhanshreeA closed this as not planned Won't fix, can't repro, duplicate, stale Nov 21, 2024
@github-project-automation github-project-automation bot moved this from On Hold to Done in Ersilia Model Hub Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed medium Useful issue or future roadmap, and needs attention
Projects
Archived in project
Development

No branches or pull requests

3 participants