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

Wizard rewrite #54

Merged
merged 43 commits into from
Oct 28, 2021
Merged

Wizard rewrite #54

merged 43 commits into from
Oct 28, 2021

Conversation

dwhswenson
Copy link
Member

This is a pretty significant rewrite of the wizard, introduced in #41. Main goals here:

  • Use new plugin infrastructure to make this more modular.
  • Wrap things from compiling (#43) whenever possible (instead of duplicating code).
  • Make it much easier to set up a wizard once a compiler plugin has already been written.
  • Better support for interactive help and other commands.

As with #43, this is only partially complete. It goes up to the state definitions. Things above that (networks, etc.) still work, but haven't been migrated to the new approach.

I think I'll merge the first draft of the wizard, then finish
the YAML parsing, then come back to the this. The correct way
to manage this is to make the Wizard wrap around the YAML parser
as much as possible -- just reuse the InstanceBuilders from that.

The we can finish the pluggable version of the wizard.
Still having an issue that periodic volumes aren't being created
correctly
@dwhswenson dwhswenson mentioned this pull request Oct 7, 2021
@dwhswenson dwhswenson added the enhancement New feature or request label Oct 7, 2021
@codecov
Copy link

codecov bot commented Oct 8, 2021

Codecov Report

Merging #54 (1399860) into main (f8ed965) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              main       #54    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           45        51     +6     
  Lines         1766      2113   +347     
==========================================
+ Hits          1766      2113   +347     
Impacted Files Coverage Δ
paths_cli/plugin_management.py 100.00% <ø> (ø)
paths_cli/wizard/joke.py 100.00% <ø> (ø)
paths_cli/commands/wizard.py 100.00% <100.00%> (ø)
paths_cli/compiling/volumes.py 100.00% <100.00%> (ø)
paths_cli/wizard/cvs.py 100.00% <100.00%> (ø)
paths_cli/wizard/engines.py 100.00% <100.00%> (ø)
paths_cli/wizard/helper.py 100.00% <100.00%> (ø)
paths_cli/wizard/load_from_ops.py 100.00% <100.00%> (ø)
paths_cli/wizard/openmm.py 100.00% <100.00%> (ø)
paths_cli/wizard/parameters.py 100.00% <100.00%> (ø)
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8ed965...1399860. Read the comment docs.

@dwhswenson
Copy link
Member Author

This is ready for review and comment. I will leave it open for at least 120 hours, merging no earlier than Sun 31 Oct 18:00 GMT (14:00 my local).

I haven't cleared the CodeClimate complaints so that they'll be available to assist review. Here are my thoughts on those. They fall into 3 categories:

  1. Cognitive complexity greater than 5, less than 10. I generally don't worry about this -- 5 is an ideal target, but not a hard maximum for me.
  2. Too many lines of code. The files where this is happening are things I don't think I want to split up (especially plugin_classes.py), though I'd be open to suggestions if there's a better way to manage this. Some of wizard.py will probably be removed as I refactor to reduce code duplication.
  3. Too many arguments (frequently for __init__). I'm using a software design style where these classes are process templates, so there are, in fact, many parameters. Many of those parameters are just what the wizard is supposed to say different stages of the process. The idea here is that, even with many parameters, it's extremely straightforward for a user to create a plugin. Look at the OpenMM plugin and the CVDefinedVolume plugin for examples -- when most of the work already exists in one of the "compiling" plugins, all you have to do is fill in what the Wizard is supposed to say. I think it is better to have a template like this, with a lot of parameters, than to ask developers to write all the code that interacts with the wizard as well (although that is also an option, and I do that when the mapping to compiler plugins isn't as straightforward.)

@dwhswenson dwhswenson changed the title [WIP] Wizard rewrite Wizard rewrite Oct 26, 2021
@dwhswenson
Copy link
Member Author

Note: I've posted issue #57 with a list of known issues that need to be improved in the wizard. I'm cutting off additional functionality in this PR where it is now, since it's already too large. The topics in #57 can be part of smaller, more manageable PRs for review.

Copy link
Member

@sroet sroet left a comment

Choose a reason for hiding this comment

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

Mostly style/leftover debug code LGTM otherwise

paths_cli/commands/wizard.py Outdated Show resolved Hide resolved
paths_cli/commands/wizard.py Show resolved Hide resolved
paths_cli/tests/compiling/test_volumes.py Outdated Show resolved Hide resolved
paths_cli/tests/wizard/test_cvs.py Outdated Show resolved Hide resolved
paths_cli/tests/wizard/test_helper.py Outdated Show resolved Hide resolved
paths_cli/wizard/volumes.py Outdated Show resolved Hide resolved
paths_cli/wizard/volumes.py Outdated Show resolved Hide resolved
paths_cli/wizard/volumes.py Outdated Show resolved Hide resolved
paths_cli/wizard/wizard.py Outdated Show resolved Hide resolved
paths_cli/wizard/wizard.py Show resolved Hide resolved
dwhswenson and others added 6 commits October 27, 2021 07:38
Co-authored-by: Sander Roet <sanderroet@hotmail.com>
A couple imports were confused and thought that test_helper
was a module for things that are used in testing, instead of a
module to test helper.py. When the import of mock_wizard into
test_helper was removed (not needed) the transitive imports led
to errors.
Co-authored-by: Sander Roet <sanderroet@hotmail.com>
@dwhswenson
Copy link
Member Author

@sroet : Thanks for the first round of review -- now this is ready for a second round. I think that all issues you brought up have been addressed except for the use of the term "subvolume", on which I'm open to suggestions for improvements.

Also, please let me know your thoughts on the CodeClimate issues and my justifications for ignoring them. This is the sort of thing where I really appreciate someone else looking over the code and telling me if the CodeClimate issues are actually causing readability problems (and maybe suggest ways around it if they are).

@dwhswenson dwhswenson requested a review from sroet October 28, 2021 14:08
@sroet
Copy link
Member

sroet commented Oct 28, 2021

@sroet : Thanks for the first round of review -- now this is ready for a second round. I think that all issues you brought up have been addressed except for the use of the term "subvolume", on which I'm open to suggestions for improvements.

I would go for "input volume"

Also, please let me know your thoughts on the CodeClimate issues and my justifications for ignoring them. This is the sort of thing where I really appreciate someone else looking over the code and telling me if the CodeClimate issues are actually causing readability problems (and maybe suggest ways around it if they are).

The reasoning for ignoring these codeclimate issues seems fine to me.

I will not have any reliable internet until monday (going to a cabin), but I can have another look after I am back if it is still required ;)

@dwhswenson
Copy link
Member Author

I will not have any reliable internet until monday (going to a cabin), but I can have another look after I am back if it is still required ;)

If everything else is resolved, I'll permit the CodeClimate issues and make the change regarding "subvolume" (perhaps "input volume you define now", given context?) Then I may go ahead and merge this -- don't think it would need your eyes again, and I'd rather have you show up on Monday to a bunch of PRs that build on the work here. 😉

@dwhswenson dwhswenson merged commit 2b6714b into openpathsampling:main Oct 28, 2021
@dwhswenson dwhswenson deleted the wizard-rewrite branch October 28, 2021 18:07
@dwhswenson dwhswenson mentioned this pull request Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants