extension/cli

[HerrHorizontal]

This is a draft for the simulate CLI. It expands the already existent CLI script to support additional choices in the defintion of the simulator object related to caching.

[pep8speaks]

Hello @HerrHorizontal! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-06-07 09:51:16 UTC

[eileen-kuehn]

@HerrHorizontal, thanks for this PR. I have one general comment before going into detail.
Did you check that none of these CLI methods is used in one of the scripts from Tabea? As long as we cannot guarantee I would recommend adding the functions that you require instead of updating the name of arguments. On this way none of the existing scripts would break.

[HerrHorizontal]

@HerrHorizontal, thanks for this PR. I have one general comment before going into detail.
Did you check that none of these CLI methods is used in one of the scripts from Tabea? As long as we cannot guarantee I would recommend adding the functions that you require instead of updating the name of arguments. On this way none of the existing scripts would break.

So the changes related to this PR did only affect the behavior in the cli/simulate.py script, which isn't used in the scripts written by @tfesenbecker. She solely relies on the custom_simulate.py script, which except for some cosmetic changes, hasn't changed at all.

Indeed, the cli/simulate.py script is meant to replace the custom_simulate.py by mimicking the behavior of the latter.

[maxfischer2781]

Got some individual suggestions in a few places. Overall, please follow Eileen's request to factor out duplicated code.

[HerrHorizontal]

Got some individual suggestions in a few places. Overall, please follow Eileen's request to factor out duplicated code.

Done

[maxfischer2781]

Thanks for these changes.

Now that the common code has been factored out of the CLI, I am having a hard time finding out what is actually different about them. As far as I can tell, static/dynamic uses --pool-files as static_pool_files/dynamic_pool_files whereas hybrid has separate --static-pool-files and --dynamic-pool-files. Is that correct?
If that's the only change, I don't think that's worth having three quasi-equivalent CLIs. Just implement only the hybrid and people can still get the static/dynamic behaviour by just not providing the other file type.

[HerrHorizontal]

Thanks for these changes.

Now that the common code has been factored out of the CLI, I am having a hard time finding out what is actually different about them. As far as I can tell, static/dynamic uses --pool-files as static_pool_files/dynamic_pool_files whereas hybrid has separate --static-pool-files and --dynamic-pool-files. Is that correct?
If that's the only change, I don't think that's worth having three quasi-equivalent CLIs. Just implement only the hybrid and people can still get the static/dynamic behaviour by just not providing the other file type.

So yes you are right. The only difference is in the pool type and the subsequent creation of the pool in the simulator (the pool_type and controller differ).

[eileen-kuehn]

Thanks for your deduplication efforts, this helps a lot.
As @maxfischer2781 already mentioned it might be great to reduce the cli to just one command as the hybrid function manages to set up the whole simulation 👍
In case you are integrating your create_simulator function back into the hybrid function you can also ignore the comment on adding type hints.

[HerrHorizontal]

Thanks for these changes.

Now that the common code has been factored out of the CLI, I am having a hard time finding out what is actually different about them. As far as I can tell, static/dynamic uses --pool-files as static_pool_files/dynamic_pool_files whereas hybrid has separate --static-pool-files and --dynamic-pool-files. Is that correct?
If that's the only change, I don't think that's worth having three quasi-equivalent CLIs. Just implement only the hybrid and people can still get the static/dynamic behaviour by just not providing the other file type.

In this case we don't need the group (static, dynamic, hybrid) anymore, aren't we? We could just work with a single command.

[eileen-kuehn]

I don't know if you want another review already.
From my point of view the code is good to go now 👍

[HerrHorizontal]

Thanks for these changes.
Now that the common code has been factored out of the CLI, I am having a hard time finding out what is actually different about them. As far as I can tell, static/dynamic uses --pool-files as static_pool_files/dynamic_pool_files whereas hybrid has separate --static-pool-files and --dynamic-pool-files. Is that correct?
If that's the only change, I don't think that's worth having three quasi-equivalent CLIs. Just implement only the hybrid and people can still get the static/dynamic behaviour by just not providing the other file type.

In this case we don't need the group (static, dynamic, hybrid) anymore, aren't we? We could just work with a single command.

I have further simplified the CLI by getting rid of the unnecessary subcommand. @eileen-kuehn, @maxfischer2781 could you please review?

[eileen-kuehn]

Good to go 👍