Added pybamm-cookiecutter CLI

[santacodes]

Additions

• Added a basic cli with an optional --path argument defining the path generated project should reside within.
• Added basic documentation for cli usage in cli.py and README.md.

Removals

• Removed models, entry points, and parameter sets from the project as they are now populated in the template.
• Removed the project-tests from noxfile and updated workflow to only test the template.
• Removed the entry points from pyproject.toml and pybamm as a dependency in the project.

Usage

To test this, check out this branch and inside the repository and do a local pipx installation on your machine.

git clone https://github.com/santacodes/pybamm-cookiecutter -b cli
cd pybamm-cookiecutter/
pipx install . 

After installation, the CLI will be available systemwide and can be accessed using the pybamm-cookiecutter command.
For help references add the -h or --help flag, e.g. pybamm-cookiecutter --help which would prompt with all the available arguments.

Sub-task #26

[agriyakhetarpal]

This looks nice, thanks, @santacodes! Since the files (for the models and the entry point handler) from the project have been removed, I think we can remove references to them from our bundled licenses (and add them to the generated project's bundled licenses if they are not present).

[Saransh-cpp]

@santacodes could you merge main here?

[santacodes]

@santacodes could you merge main here?

I did merge it, I think the merge commit got hidden within the resolved merge conflicts commit here. Sorry if it didn't show up, I should have probably rebased it but I resolved the merge conflicts locally and pushed them here as they were a bit complicated to resolve and there was no web editor option with this one.

[agriyakhetarpal]

This is looking good, thanks, @santacodes! Here are some of my suggestions besides the code review that are better provided in text:

1. Installing and running it displays the message No git tags found in template, using HEAD as ref – this is marked in red, which might prompt users/newcomers to think that something is wrong. Is there a way to disable this message or make it clearer? That said, I don't think this is coming from any of our changes, though.
2. The CLI has a problem where it truncates long sentences, at least on macOS – I noticed that the sentence was terminated before completion as follows: "such as `import p"
3. There's no option to go back if I made an error while typing things out – does copier support undo functionality?
4. I think we need better exception handling. If I terminate the program in between by pursuing a KeyboardInterrupt (Command + D on my machine), it prints out "Error caused by an exception:" followed by a long list of metadata that was being used to generate the project, stopped midway. Comparing this with the copier CLI, this is just "Execution stopped by user", which is more elegant. Also, Is there a way to distinguish a keyboard interrupt from an actual error?
5. Also, in the previous point, a keyboard interrupt midway into creating a project returns an exit code of 0, indicating success, instead of a non-zero exit code to indicate failure – I think it should be 0 for successful completion, 1 for an error in the project generation, and 2 for a keyboard interrupt, and higher for any other non-standard error.

[santacodes]

This is looking good, thanks, @santacodes! Here are some of my suggestions besides the code review that are better provided in text:

1. Installing and running it displays the message No git tags found in template, using HEAD as ref – this is marked in red, which might prompt users/newcomers to think that something is wrong. Is there a way to disable this message or make it clearer? That said, I don't think this is coming from any of our changes, though.

I think this could be resolved by providing a vcs_ref via the command line or the copier API, I will look into that.

2. The CLI has a problem where it truncates long sentences, at least on macOS – I noticed that the sentence was terminated before completion as follows: "such as `import p"

Could you please attach a screenshot of some kind it's hard for me to picture it 😅 .

3. There's no option to go back if I made an error while typing things out – does copier support undo functionality?

I think it does not, looking at the documentation. Maybe I could try to come up with some kind of a hacky way to do it something like recording a particular keystroke for example ctrl+z and then with the cached recorded user inputs, generate the project using the copier API again which would probably continue from where the user left it off. I think that would be something unreliable though, but if we are doing it, this might just be one of the ways to do it.

4. I think we need better exception handling. If I terminate the program in between by pursuing a KeyboardInterrupt (Command + D on my machine), it prints out "Error caused by an exception:" followed by a long list of metadata that was being used to generate the project, stopped midway. Comparing this with the copier CLI, this is just "Execution stopped by user", which is more elegant. Also, Is there a way to distinguish a keyboard interrupt from an actual error?

I agree with that, I will add better exception handling in the CLI and try to cover most of the endpoints.

5. Also, in the previous point, a keyboard interrupt midway into creating a project returns an exit code of 0, indicating success, instead of a non-zero exit code to indicate failure – I think it should be 0 for successful completion, 1 for an error in the project generation, and 2 for a keyboard interrupt, and higher for any other non-standard error.

For that, I think we could just wrap the exception handling to KeyBoardInterrupt rather than exit codes.

[agriyakhetarpal]

Yes, letting Python handle the KeyBoardInterrupt will let it use its related exit code, I think. Let's skip the undo functionality for now (or you could try your idea – if the implementation is minimal, that's great).

For the screenshot, here you go:

which indicates that the prompt for the second question was cut off. Maybe there's a limit on the number of characters in that can be configured, or maybe it's a hard limit and we can reword the sentence to make it shorter?

[agriyakhetarpal]

Also, if we were to run copier internally in a subprocess.call() – i.e., sys.exit(subprocess.call(argv)), that should resolve the problem with the exit codes automatically. We'll have to switch to using the copier CLI instead of the API, though.

[santacodes]

Also, if we were to run copier internally in a subprocess.call() – i.e., sys.exit(subprocess.call(argv)), that should resolve the problem with the exit codes automatically. We'll have to switch to using the copier CLI instead of the API, though.

I think it might be a bad idea to use the subprocess library to access the copier CLI, the exit code 0 on KeyboardInterrupt is predefined here in the copier code base even for the CLI. Maybe we should aim for a better exception handling in the pybamm-cookiecutter CLI to cover all the cases.

[agriyakhetarpal]

I would say we should use what brings simpler code to write (and test) – my suggestion is not about handling KeyboardInterrupts, since there are other exceptions to handle. If a subprocess call for us can directly inherit this behaviour from copier, we might as well do so (since copier is not going to deprecate its CLI anytime soon).

[santacodes]

I would say we should use what brings simpler code to write (and test) – my suggestion is not about handling KeyboardInterrupts, since there are other exceptions to handle. If a subprocess call for us can directly inherit this behaviour from copier, we might as well do so (since copier is not going to deprecate its CLI anytime soon).

Umm I think you misunderstood me, you should perhaps look at this copier CLI source code, but in brief, the CLI handles only three exceptions out of which 2 are copier specific errors which are already handled in the Worker class. So basically how all this works is the copier CLI takes in all the arguments from the command line, parses it and passes the command line arguments to a Worker instance, and does the run_copy() method here. In our CLI we are performing the same thing, except we are directly performing the run_copy method of the Worker class like defined here in the source code. So the user prompts which you are concerned about are actually generated by the Worker class and not the copier CLI, which is also why we see the same prompt UI when we use the copier command or access it through the run_copy() command. I just feel we would just increase a layer of abstraction with a subprocess call just for a KeyboardInterrupt exception because right now the pybamm-cookiecutter CLI works like -

pybamm-cookiecutter CLI -> Worker.run_copy
while your proposed method would do something like -
pybamm-cookiecutter CLI -> copier CLI -> Worker.run_copy

And even the word trimmings you mentioned earlier shouldn't be much of an issue because they only get trimmed after a user answers the prompt if I am not wrong, I tested that locally as well. Overall, I think we should be good with handling the KeyboardInterrupt error in our CLI as that might be the only error we might run into from our side because the Worker class directly handles every other exception case in the core library and not the copier CLI. I hope my explanation made sense😅, feel free to correct me haha.

[agriyakhetarpal]

Thanks for the explanation and the cross-links to the code, @santacodes – makes sense to me! Yes, there's no need for extra abstractions, then. Please let us know when this is ready for review again.

[Saransh-cpp]

Thanks, @santacodes! This looks really good. A couple of comments below -

[Saransh-cpp]

Thanks for working on this, looks amazing, @santacodes!

Good practice tip: You should revert the commit where an unwanted file (in this case - _version.py) was added instead of deleting the file in another commit. Adding and deleting the file still leaves it in the git history. We don't require reverting here as we will "squash and merge" the PR, so the git history within this PR will be lost anyways.

[Saransh-cpp]

Could you also add the CI badge in the README -

[arjxn-py]

Thanks @santacodes, a couple of comments below

[agriyakhetarpal]

Thanks, @santacodes! I tested out the CLI exceptions and this looks good to go now. I do have one suggestion, so I'm not approving this at the moment – happy to do so once it's fixed. It's about license compliance (xref #43) – I built the sdist and wheel, and while both do contain the relevant LICENSE files, this is the content of the project's (pybamm-cookiecutter's) bundled LICENSES file, which doesn't seem to make much sense:

$ cat LICENSES-bundled.txt

This project and source distributions bundle several libraries that are
compatibly licensed.


Name: PyBaMM
Files: - src/{{ project_slug }}/parameters/input/Chen2020.py
       - src/{{ project_slug }}/models/input/BasicReservoir.py
       - src/{{ project_slug }}/models/input/SPM.py
       - src/{{ project_slug }}/entry_point.py
License: BSD-3-Clause

because pybamm-cookiecutter doesn't contain these files anymore, and pybamm-cookiecutter won't know about what {{ project_slug }} represents either until a project is generated. This should be moved to the generated project's LICENSES-bundled.txt.jinja file (which was outdated), since the generated project is where the files now exist. I think we can remove the LICENSES-bundled.txt file from pybamm-cookiecutter entirely, since we are now not using any third-party code.

[agriyakhetarpal]

Thanks, @santacodes!