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

Command line runs #725

Merged
merged 2 commits into from
Jun 8, 2020
Merged

Command line runs #725

merged 2 commits into from
Jun 8, 2020

Conversation

bettbra
Copy link
Contributor

@bettbra bettbra commented Jun 6, 2020

Add a yarn target for running the model via the command line.

Description

These changes allow the model to be run via the command line via a yarn cli call. Two arguments are required: --scenario and --out.

Impacted Areas in the application

The changes are all pretty trivial. Some command line parsing is done via yargs in src/algorithms/cli.ts (and so a dependency is introduced on the yargs package in package.json).

Testing

  1. Copy the example scenario parameters shown on docs/comand_line.md into a file scenario.json.
  2. At the command line, execute yarn cli --help to see the usage message.
  3. At the command line, execute yarn cli --scenario=scenario.json --out=output.json.
  4. Confirm that output.json was created by the run and has reasonable-looking output.

Additional Notes

  • We are using these mods as we do some grid searching across input parameters.
  • I recognize this is our first pull request to your project. We hope we have adhered to the contribution guidelines, but of course let us know how we can modify the changes to better suit the project. And equally, zero worries if you just want to reject the pull request.
  • Thank you for your hard work on this project and for making it public so that we can all benefit and contribute.

@vercel
Copy link

vercel bot commented Jun 6, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/covid19-scenarios/covid19-scenarios/r6ukndfu4
✅ Preview: https://covid19-scenarios-git-fork-bettbra-cli.covid19-scenarios.now.sh

@codeclimate
Copy link

codeclimate bot commented Jun 6, 2020

Code Climate has analyzed commit 0bc426b and detected 0 issues on this pull request.

View more on Code Climate.

@bettbra bettbra marked this pull request as draft June 6, 2020 20:13
@ivan-aksamentov
Copy link
Member

ivan-aksamentov commented Jun 7, 2020

@bettbra Hi Bradley,
Thanks! I should probably note that at this point we prioritize the app and we don't officially support the CLI. Personally, I never tried to run it and don't know if it runs correctly. Theoretically it should :) That being said, it wouldn't hurt to have a CLI if community wants it.

However, before merging, I need to ask the folks at GitHub/Microsoft who originally contributed it in #689. @adityasharad Could you please review the changes and see if it is okay for your use-case? I'll be happy to merge then.

@bettbra Could you please tell us more about your project, so that we could take this information into account when planning the improvements of the algorithm?

Any additional improvements are welcome, including any of the items listed in #663

@ivan-aksamentov ivan-aksamentov marked this pull request as ready for review June 7, 2020 12:46
@bettbra
Copy link
Contributor Author

bettbra commented Jun 7, 2020

Hi Ivan,

Thanks for your reply. Right, I should have checked to try to infer if another team was also using the CLI. I suspect the changes here won't work for them (e.g., @adityasharad) as they already have dependencies on the existing form, but maybe we can coordinate. For us it's convenient to be able to control all parameters---scenario, age, severity---at the command line, meaning not only the first set. So really the change here is focused on adding the last two (age, severity) and using yargs for argument parsing purely for convenience.

I also see a bunch of eslint warnings that I'll fix; forgive me, didn't realize those were getting run. I'll eliminate all the warnings to adhere to the project conventions.

As for what we're using the project for, we're trying to vary R_0 using mobility data, very similar in spirit to #715. We execute large numbers of runs as we assess sensitivities to various input parameters, doing this on the command line in a large gridded compute environment. The UI is fantastic and we use it for quick sanity checks, but our heavy lifting is done via the CLI.

Apologies for the long reply. If the community would favor making the CLI a supported interface we'd be enthusiastic supporters and commit to helping maintain it. Thanks again @ivan-aksamentov. I'll await additional overall feedback and at least fix those warnings as mentioned.

--Brad

@ivan-aksamentov
Copy link
Member

ivan-aksamentov commented Jun 7, 2020

@bettbra

For us it's convenient to be able to control all parameters---scenario, age, severity---at the command line

Yes. That is probably the most useful interface for the community.

Feel free to propose the improvements to the interface that are useful for you. For example we could make every parameter to be exposed from both files and command line, and command line would take precedence. This way one could have a base config and vary a given parameter. We may also read json from stdin, so that users could pipe the autogenerated configs through the shell or exec.

I also see a bunch of eslint warnings

Note that the current eslint config is mostly tailored for the React app, so some of these warning may not necessarily make much sense for a Node script. In this case, feel free to either (1) add an eslint-ignore comment for the file or (2) add an item in the override section of the eslint config file or (3) .eslintignore the cli file/directory and create a separate config. Whichever suits your needs the best.

As for what we're using the project

Wow, that sounds cool! Do you have a link?

helping maintain it

That would be fantastic! Feel free to pick any if the items in #663 and add yours too. We are looking forward to any feedback and contributions.

I guess currently you have to pull the project from GitHub in order to use it, maybe as a submodule or similar. Would it be useful if we create and NPM package with a CLI and programmatic Node.js interface? We could automate the build and publishing as well. One advantage is more convenient versioning and reduction of breakage, because we are currently not tracking how our changes affect the CLI at all.

Copy link
Contributor

@adityasharad adityasharad left a comment

Choose a reason for hiding this comment

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

Lovely! I think this will work nicely with the way we're currently using the model, and is certainly better than my initial version. Thanks @ivan-aksamentov for keeping me in the loop.

Minor suggestions about typings, otherwise looks good. I defer to Ivan on the eslint warnings - some of them look easy to fix but I have no strong feelings about suppressing vs fixing them.

@bettbra we (on the GitHub side) would love to hear more about your work on running and evaluating different models.

*/
function getSeverity(inputFilename: string | undefined) {
if (inputFilename) {
const data = readJsonFromFile<any>(inputFilename)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can type this as SeverityDistributionData instead of any. That type can be added to the import from ./types/Param.types.

*/
function getAge(inputFilename: string | undefined, name: string) {
if (inputFilename) {
const data = readJsonFromFile<any>(inputFilename)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can type this as AgeDistributionData instead of any. That type can be added to the import from ./types/Param.types.

@rneher
Copy link
Member

rneher commented Jun 8, 2020

this does look like a very useful direction and we might consider ditching our parallel implementation in python for model fitting.

@ivan-aksamentov
Copy link
Member

@adityasharad Thanks for reviewing.

@adityasharad @bettbra I will merge and then fix the kinks on master. Let me know if there's anything to ammend or what other improvements you have in mind.

@ivan-aksamentov ivan-aksamentov merged commit aebf3ee into neherlab:master Jun 8, 2020
@bettbra
Copy link
Contributor Author

bettbra commented Jun 8, 2020

Thanks all for the input. I fixed the eslint kinks on my branch without changing the eslint settings. I can open another pull request for it if you'd like; apologies for the delay. Thanks @adityasharad and @ivan-aksamentov for your feedback.

@bettbra
Copy link
Contributor Author

bettbra commented Jun 8, 2020

Oh, and I should have commented @rneher: We're grateful for any direction you want to take the project that allows command line running, or more broadly programmatic modification of parameters. If this does the trick for the community, super, but happy to switch to any other language or methodology.

Thanks again for all your hard work on this.

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.

5 participants