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

Merged card command with catalog. #1264 #1285

Merged
merged 12 commits into from
Oct 9, 2024

Conversation

teddyCodex
Copy link
Contributor

@teddyCodex teddyCodex commented Oct 3, 2024

Thank you for taking your time to contribute to Ersilia, just a few checks before we proceed

  • [✅] Have you followed the guidelines in our Contribution Guide
  • Have you written new tests for your core changes, as applicable?
  • [✅] Have you successfully ran tests with your changes locally?

Description

Merged card command with catalog command while disabled existing ersilia card command.
This would allow a user add an optional --card flag to the catalog command enabling them view metadata about the particular model card requested.
The catalog command should still work as expected (without the --card flag)- providing a list of all available models in the Model Hub

Status

Commented out card.cmd to disable existing card command.
Implemented new card flag in catalog.py and updated catalog function to use the card command.
Removed unused ModelSearcher Class.

Is this pull request related to any open issue? If yes, replace issueID below with the issue ID

Closes #1264

@Malikbadmus
Copy link
Contributor

@teddyCodex , you can go ahead and deregister the card command in cmd.py and remove the card.py file.

@Malikbadmus
Copy link
Contributor

@DhanshreeA , I've reviewed the PR and suggested a few changes. I've also tested the code, and everything is working as expected. It looks good to be merged.

@teddyCodex
Copy link
Contributor Author

@teddyCodex , you can go ahead and deregister the card command in cmd.py and remove the card.py file.

Noted @Malikbadmus Will do so shortly.

@DhanshreeA
Copy link
Contributor

Thanks @Malikbadmus and @teddyCodex !

@teddyCodex
Copy link
Contributor Author

@teddyCodex , you can go ahead and deregister the card command in cmd.py and remove the card.py file.

@Malikbadmus @DhanshreeA done and pushed.

@DhanshreeA
Copy link
Contributor

@teddyCodex this generally looks good, requesting some code cleanliness changes.

- Changed the '--card' option to a flag with a default value of 'False'.
- Added 'model' as a positional argument to the 'catalog' command and made it optional.
- Implemented logic to handle the case where 'card' is provided without a 'model'.
- Added error handling to notify user if no metadata is found for the specified model when using the 'card' flag.
@teddyCodex
Copy link
Contributor Author

teddyCodex commented Oct 5, 2024

Hi @DhanshreeA All changes implemented.
Also implemented a logic to handle a situation where metadata isn't found for the specified model (or model doesn't exist)
all related commands work as expected.

ersilia catalog will print all models
ersilia catalog -- card will print an error
ersilia catalog --card xxxxx will print an error
ersilia catalog --card eos935d will print metadata for eos935d

@teddyCodex teddyCodex requested a review from DhanshreeA October 5, 2024 11:39
@teddyCodex teddyCodex requested a review from Malikbadmus October 6, 2024 23:09
@teddyCodex
Copy link
Contributor Author

Hi @Malikbadmus @DhanshreeA had a chance to give this another look?

@teddyCodex teddyCodex requested a review from DhanshreeA October 8, 2024 15:57
@DhanshreeA
Copy link
Contributor

Fantastic work, thanks @teddyCodex!

@DhanshreeA DhanshreeA merged commit a5a2f37 into ersilia-os:master Oct 9, 2024
18 checks passed
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.

🐈 Task: Merge card command with catalog.
3 participants