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.

Sign up for GitHub

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

Yaml format for CLI input #583

Merged
merged 18 commits into from
Nov 17, 2023
+355 −40
Merged

Yaml format for CLI input #583

Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
637c7f6
[skip ci]
richardjgowers Oct 24, 2023
21a0df6
adds yaml reader for cli options input
richardjgowers Oct 25, 2023
17e5026
change plan_rXfe_network_main functions to expect list of mappers
richardjgowers Oct 26, 2023
d4e3261
move settings yaml parsing to be parameter
richardjgowers Oct 26, 2023
4ef7ab1
add YAML_OPTIONS as parameter
richardjgowers Oct 26, 2023
7ad79c7
add YAML_OPTIONS to plan_rbfe_network
richardjgowers Oct 26, 2023
53fc5b1
cli: add interpretation of settings yaml
richardjgowers Oct 26, 2023
675623c
cli: enable settings yaml reading in plan_rbfe_network
richardjgowers Oct 26, 2023
900e43b
cli: some extra documentation on load_yaml
richardjgowers Oct 30, 2023
4ec89e0
cli: fix tests to account for normalisation
richardjgowers Oct 30, 2023
912720a
cli: add test for custom rbfe yaml usage
richardjgowers Oct 30, 2023
5f5d619
cli: update plan_rhfe to allow yaml options
richardjgowers Oct 30, 2023
90bfae8
cli: use v1/2 compatible Pydantic
richardjgowers Oct 31, 2023
f78762f
cli: fixup pydantic v1/2 compat
richardjgowers Nov 1, 2023
aab5620
remove draft yaml idea
richardjgowers Nov 1, 2023
0d57408
cli: centralise parsing of yaml options and defaults
richardjgowers Nov 14, 2023
16cae4b
add pyyaml to deps
richardjgowers Nov 14, 2023
a1fb632
Merge branch 'main' into cli_yaml_draft
IAlibay Nov 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions openfecli/commands/input.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# things behind a "method" key have to match up to a known thing in openfe

network:
method: plan_radial_network
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these going to be required? Like will there be some defaults if it is blank, or do we require some (or all) to have values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think everything in the file is optional. It's just to augment the command line plan_rbfe_network.

scorer: default_lomap_score

# Question:
# use settings here, to match Protocol
Copy link
Member

Choose a reason for hiding this comment

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

I like the "settings" nest, since a) allows for further expansion for non-settings keys if they are ever needed, b) it's just visually pleasing to look at.

Not a super strong vote though.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on settings nesting

Copy link
Member

Choose a reason for hiding this comment

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

I honestly don't remember which approach was easier for a JSON Schema. I think the not nested version is a little easier: you define a single subschema with a literal for method and the appropriate list of settings keys. With nested settings, I think you have to define the subschema with the method literal and point another subschema for the settings. That would make documentation, e.g., as generated by sphinx-jsonschema, a little ugly. TBH, it is really just a question of which is easier with whatever tool we'll do to generate the JSON schema (do not go down the road of rolling your own).


mappers:
- method: LomapAtomMapper
settings:
timeout: 60
- method: LomapAtomMapper
settings:
timeout: 120

# OR not nesting settings, anything without the "method" key gets put into kwargs
Copy link
Member

Choose a reason for hiding this comment

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

Either one works from a JSON Schema standpoint. I'd vote to start by requiring it to be a list and add handling for non-list later, just as a matter of do the simple thing first.


mappers:
- method: LomapAtomMapper
timeout: 60
- method: LomapAtomMapper
timeout: 120


# Question:
# is mapper*s* always a list and mapper always a single item
# or do we allow mapper to define either a list or single item behind it?

Copy link
Contributor

Choose a reason for hiding this comment

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

If there is more than one mapper, how are things divided up? Like if there are 2 mappers, do we just create 2 mappings for each leg? or do you control which leg gets which mapper (if this is out of scope for what you are doing, then ignore).

I think I like:

mappers:
  - method: LomapAtomMapper
    settings:
      timeout: 60

and

mappers:
  - method: LomapAtomMapper
    settings:
      timeout: 60
  - method: LomapAtomMapper++
    settings:
      timeout: 60

so that we either have a list or a single one, but it always uses the mappers key even if there is one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way multiple mappers works is already define in the plan_radial_network etc functions. see: https://github.com/OpenFreeEnergy/openfe/blob/main/openfe/setup/ligand_network_planning.py#L99

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's more aesthetically, is the spec that we allow Union[AtomMapper, list[AtomMapper]] to mapper? This would match the above signature I guess

mapper:
method: LomapAtomMapper
settings:
timeout: 120


protocol:
method: OpenMM_RFE
settings:
alchemical_sampler_settings:
n_repeats: 5
simulation_settings:
production_length: 11 ns