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

🐛 Bug: CLI commands improvements #1486

Open
GemmaTuron opened this issue Jan 3, 2025 · 6 comments · Fixed by #1495
Open

🐛 Bug: CLI commands improvements #1486

GemmaTuron opened this issue Jan 3, 2025 · 6 comments · Fixed by #1495
Assignees
Labels
bug Something isn't working

Comments

@GemmaTuron
Copy link
Member

Describe the bug.

I have tested all the CLI commands listed in here: https://ersilia.gitbook.io/ersilia-book/ersilia-model-hub/developer-docs/command-line-interface. The documentation gives a good high level overview yet small modifications are needed to facilitate user guidance. As a general comment, it is not clear when a flag is required (for example run -i) and when is optional. To facilitate the revision of the CLI, I noted down everything I have found, both larger or smaller issues, reporting everything for completion without any prioritisation/level of criticality:

  • catalog -f only works if a .csv file is asked. Otherwise it creates an empty file, for example a .txt. If only .csv is allowed, this should be specified in the documentation. From what I see here the only options are -csv or -tsv.
  • the --more flag shows some Model Source empty, but I think this is more to do with the models than the command. Models are eos7jio and eos7w6n
  • I don't know where the --hub flag is reading from but we should have a filter to avoid showing internal models like eos0t04 (test) or eos30d7 (another test?)
  • The --card command documentation should specify --card [MODEL] or sth as I could also do catalog model_id --card maybe if I am not familiar with Ersilia's infrastructure
  • Why the example command adds [?MODEL] as an option? Adding a Model is not optional right? Is the ? a typo?
  • On the example -f specify which kind of files can be saved. In this case it did work for csv and txt
  • example --predefined returns randomly fetched molecules if the example file is not available, without notifying the user. A notification at least should be added, and/or explanation of this behavior in the documentation
  • fetch from_bentoml: this sentence For that to happen, the model source needs to follow a specific structure. is very vague. Explain better or link to somewhere showing this structure. Same for -from_fastapi
  • What does the [OPT] stand for in the fetch from_benotml and fetch from_fastapi options?
  • run command: why is [?OPT] indicated with ? the run command without additional flags does not work.
  • run --batch: specify in the docs what is the default batch size. Related to the above, it is not clear if --batch is required. Only -i is needed, so specify that in the docs.
  • run --table: Error: No such option: --table Did you mean --as-table? This popped up when trying to use the command, please revise. This command: ersilia run -i example.csv --as-table did not give error but did not print anything (model eos4u6p)
  • serve --lake: I might be confused but we are not saving predictions anywhere now? even in a local lake?

Describe the steps to reproduce the behavior

No response

Operating environment

Ubuntu 24.04 LTS

@GemmaTuron GemmaTuron added the bug Something isn't working label Jan 3, 2025
@GemmaTuron
Copy link
Member Author

and the test and inspect commands are not documented in the CLI

@DhanshreeA DhanshreeA moved this from On Hold to In Progress in Ersilia Model Hub Jan 7, 2025
@DhanshreeA
Copy link
Member

DhanshreeA commented Jan 7, 2025

@GemmaTuron thanks a lot for this issue and the last mile work on this front. From what you have posted above, some things are clearly issues, and some other things require clarification.

  1. I think we should support both .csv, and .tsv, while clearly adding this in the documentation. In the case that the user adds a different extension than these two, so for example, catalog.txt, we should still serialize it but by default we should still keep it a comma separated file, the extension notwithstanding.
  2. The --more command should never show ModelSource as empty, unless something got corrupted in the entries for these models in the EOS directory.
  3. I fully agree, I'll add a filter for this.
  4. I agree - the documentation for --card needs to be changed.
  5. The example command can work BOTH with a model currently served in the session where the example command is run, AND explicitly for any specified model. In the case when a model is neither specified explicitly, nor is it running in the session, then the example command will raise an error and exit. So, the documentation is correct and doesn't contain a typo. Refer the output below:
(ersilia) ☁  ersilia-project  ersilia -v example -n 5 -f input.csv
23:33:53 | DEBUG    | No session exists
No model found. Please specify a model or serve a model in the current shell.
  1. Noted, I'll update the documentation for example to specify file types.
  2. Noted, makes sense, I'll add a log in the CLI to inform the user that we are randomly sampling inputs.
  3. Got it, I'll add links for the old eos-template, and the new eos-template, and link relevant Ersilia Pack related issues for now.
  4. The OPT with --with_fastapi, and --with_bentoml applies because one might want to use --overwrite/--reuse specifically for the conda environment. Beyond that, there is no other use case for this. I think this might be worthwhile calling out in the documentation.
  5. With the run command, the ? is a typo, you're right.
  6. Noted for both batch as a non required option, and default batch size.
  7. I'll look into this as_table issue.
  8. Okay, I am bit conflicted about this - technically it should work with older versions in ersilia like 0.1.34 and below where isaura can be installed, but does not for newer versions. I am in favor of removing the flag entirely, second best option is to add a deprecation notice for this flag.

@GemmaTuron
Copy link
Member Author

thanks @DhanshreeA !

I agree with the suggestions, for 13 maybe a discussion about what we do with the lakes is needed once we complete this sprint on model testing and refactoring, coupled with Splunk's next project suggestions. Let's leave as is and revisit it once we have more time.

@DhanshreeA
Copy link
Member

@GemmaTuron actually the ? is not incorrect with the run command. Only the -i/--input is required, all remaining options are optional indeed. 😅

@GemmaTuron
Copy link
Member Author

Hi @DhanshreeA

Maybe I am missing something but if you have run [?OPT] to me it is not indicating that the -i is required. As a user how do I know I need the input flag but not the output one for example?

@GemmaTuron GemmaTuron reopened this Jan 14, 2025
@DhanshreeA
Copy link
Member

@GemmaTuron okay so I have updated the documentation on GitBook to say, for the run -i/--input column:

Specify the input file, which must be a CSV. Alternatively a single SMILES input can be specified. The input file should ideally have a header. This is a required flag to run a model with an input.

The last line mentions that this a required field to actually run a model. No other option mentions that, I think that's sufficient?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants