Yaml format for CLI input #583
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,179 @@ | ||||||
| # This code is part of OpenFE and is licensed under the MIT license. | ||||||
| # For details, see https://github.com/OpenFreeEnergy/openfe | ||||||
| """Pydantic models for the definition of advanced CLI options | ||||||
|
|
||||||
| """ | ||||||
| import click | ||||||
| from collections import namedtuple | ||||||
| try: | ||||||
| # todo; once we're fully v2, we can use ConfigDict not nested class | ||||||
| from pydantic.v1 import BaseModel # , ConfigDict | ||||||
| except ImportError: | ||||||
| from pydantic import BaseModel | ||||||
| from plugcli.params import Option | ||||||
| from typing import Any, Optional | ||||||
| import yaml | ||||||
richardjgowers marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| import warnings | ||||||
|
|
||||||
|
|
||||||
| PlanNetworkOptions = namedtuple('PlanNetworkOptions', | ||||||
| ['mapper', 'scorer', | ||||||
| 'ligand_network_planner', 'solvent']) | ||||||
|
|
||||||
|
|
||||||
| class MapperSelection(BaseModel): | ||||||
| # model_config = ConfigDict(extra='allow', str_to_lower=True) | ||||||
| class Config: | ||||||
| extra = 'allow' | ||||||
| anystr_lower = True | ||||||
|
|
||||||
| method: Optional[str] = None | ||||||
|
There was a problem hiding this comment. Rather than validating later on, could we just use a validator here for the allowed method names? There was a problem hiding this comment. discussed this live, it won't be possible to have a list of possible methods a priori |
||||||
| settings: dict[str, Any] = {} | ||||||
|
|
||||||
|
|
||||||
| class NetworkSelection(BaseModel): | ||||||
| # model_config = ConfigDict(extra='allow', str_to_lower=True) | ||||||
| class Config: | ||||||
| extra = 'allow' | ||||||
| anystr_lower = True | ||||||
|
|
||||||
| method: Optional[str] = None | ||||||
| settings: dict[str, Any] = {} | ||||||
|
|
||||||
|
|
||||||
| class CliYaml(BaseModel): | ||||||
| # model_config = ConfigDict(extra='allow') | ||||||
| class Config: | ||||||
| extra = 'allow' | ||||||
|
|
||||||
| mapper: Optional[MapperSelection] = None | ||||||
| network: Optional[NetworkSelection] = None | ||||||
|
|
||||||
|
|
||||||
| def parse_yaml_planner_options(contents: str) -> CliYaml: | ||||||
| """Parse and minimally validate a user provided yaml | ||||||
|
|
||||||
| Parameters | ||||||
| ---------- | ||||||
| contents : str | ||||||
| raw yaml formatted input to parse | ||||||
|
|
||||||
| Returns | ||||||
| ------- | ||||||
| options : CliOptions | ||||||
| will have keys for mapper and network topology choices | ||||||
|
|
||||||
| Raises | ||||||
| ------ | ||||||
| ValueError | ||||||
| for any malformed inputs | ||||||
| """ | ||||||
| raw = yaml.safe_load(contents) | ||||||
|
|
||||||
| if False: | ||||||
|
There was a problem hiding this comment. I think this would be good to have and also helps with typos in keys |
||||||
| # todo: warnings about extra fields we don't expect? | ||||||
| expected = {'mapper', 'network'} | ||||||
| for field in raw: | ||||||
| if field in expected: | ||||||
| continue | ||||||
| warnings.warn(f"Ignoring unexpected section: '{field}'") | ||||||
|
|
||||||
| return CliYaml(**raw) | ||||||
|
|
||||||
|
|
||||||
| def load_yaml_planner_options(path: Optional[str], context) -> PlanNetworkOptions: | ||||||
| """Load cli options from yaml file path and resolve these to objects | ||||||
|
|
||||||
| Parameters | ||||||
| ---------- | ||||||
| path : str | ||||||
| path to the yaml file | ||||||
| context | ||||||
| unused | ||||||
|
|
||||||
| Returns | ||||||
| ------- | ||||||
| PlanNetworkOptions : namedtuple | ||||||
| a namedtuple with fields 'mapper', 'scorer', 'network_planning_algorithm', | ||||||
| and 'solvent' fields. | ||||||
| these fields each hold appropriate objects ready for use | ||||||
| """ | ||||||
| from gufe import SolventComponent | ||||||
| from openfe.setup.ligand_network_planning import ( | ||||||
| generate_radial_network, | ||||||
| generate_minimal_spanning_network, | ||||||
| generate_maximal_network, | ||||||
| generate_minimal_redundant_network, | ||||||
| ) | ||||||
| from openfe.setup import ( | ||||||
| LomapAtomMapper, | ||||||
| ) | ||||||
| from openfe.setup.atom_mapping.lomap_scorers import ( | ||||||
| default_lomap_score, | ||||||
| ) | ||||||
| from functools import partial | ||||||
|
|
||||||
| if path is not None: | ||||||
| with open(path, 'r') as f: | ||||||
| raw = f.read() | ||||||
|
|
||||||
| # convert raw yaml to normalised pydantic model | ||||||
| opt = parse_yaml_planner_options(raw) | ||||||
| else: | ||||||
| opt = None | ||||||
|
|
||||||
| # convert normalised inputs to objects | ||||||
| if opt and opt.mapper: | ||||||
| mapper_choices = { | ||||||
|
There was a problem hiding this comment. Not for here, but later - it'd be great if we could define this as a dictionary somewhere else, that way we can just import that (I'm thinking about expanding this to include perses, kartograf, etc..) There was a problem hiding this comment. also won't be known ahead of time, more likely this is more a EAFP situation |
||||||
| 'lomap': LomapAtomMapper, | ||||||
| 'lomapatommapper': LomapAtomMapper, | ||||||
|
There was a problem hiding this comment. What is the advantage of having two choices here? Would it not be safer to just support the one thing? There was a problem hiding this comment. just aliases to make finding options easier |
||||||
| } | ||||||
|
|
||||||
| try: | ||||||
| cls = mapper_choices[opt.mapper.method] | ||||||
| except KeyError: | ||||||
| raise KeyError(f"Bad mapper choice: '{opt.mapper.method}'") | ||||||
|
There was a problem hiding this comment. It would be nice to add
Suggested change
Or something like that |
||||||
| mapper_obj = cls(**opt.mapper.settings) | ||||||
| else: | ||||||
| mapper_obj = LomapAtomMapper(time=20, threed=True, element_change=False, | ||||||
| max3d=1) | ||||||
|
There was a problem hiding this comment. Separate issue: @hannahbaumann did bring up the fact that There was a problem hiding this comment. this should ideally be up to date with our best practices, so sounds like we can patch this There was a problem hiding this comment. |
||||||
|
|
||||||
| # todo: choice of scorer goes here | ||||||
| mapping_scorer = default_lomap_score | ||||||
|
|
||||||
| if opt and opt.network: | ||||||
| network_choices = { | ||||||
|
There was a problem hiding this comment. @dwhswenson avert your eyes! This is the hacky job I think will initially serve us. I think having this dict-like thing being eventually much more clever is what you're envisaging, this is as far as I'm taking it. |
||||||
| 'generate_radial_network': generate_radial_network, | ||||||
| 'radial': generate_radial_network, | ||||||
| 'generate_minimal_spanning_network': generate_minimal_spanning_network, | ||||||
| 'mst': generate_minimal_spanning_network, | ||||||
| 'generate_minimal_redundant_network': generate_minimal_redundant_network, | ||||||
| 'generate_maximal_network': generate_maximal_network, | ||||||
| } | ||||||
|
|
||||||
| try: | ||||||
| func = network_choices[opt.network.method] | ||||||
| except KeyError: | ||||||
| raise KeyError(f"Bad network algorithm choice: '{opt.network.method}'") | ||||||
|
|
||||||
| ligand_network_planner = partial(func, **opt.network.settings) | ||||||
| else: | ||||||
| ligand_network_planner = generate_minimal_spanning_network | ||||||
|
|
||||||
| # todo: choice of solvent goes here | ||||||
| solvent = SolventComponent() | ||||||
|
|
||||||
| return PlanNetworkOptions( | ||||||
| mapper_obj, | ||||||
| mapping_scorer, | ||||||
| ligand_network_planner, | ||||||
| solvent, | ||||||
| ) | ||||||
|
|
||||||
|
|
||||||
| YAML_OPTIONS = Option( | ||||||
| '-s', "--settings", "yaml_settings", | ||||||
| type=click.Path(exists=True, dir_okay=False), | ||||||
| help="Path to planning settings yaml file.", | ||||||
| getter=load_yaml_planner_options, | ||||||
| ) | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be the same:
You don't need to explicitly specify default behavior. The purpose of other things changing
help(for example) is to re-use most of the behavior of the parameter (or even help string) and add something else to clarify the use in this specific case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm (we're?) not familiar with the defaults, so being explicit here is useful to know what's happening
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want to actually be explicit, please list all standard defaults for click.Option and click.Parameter, except for those overridden in
YAML_OPTIONS. Personally, I think that's way too much information, so I don't think that being explicit is useful here.In particular, please do not explicitly list anything that is just repeated from
YAML_OPTIONS, such ashelp=YAML_OPTIONS["help"]. This is not informative to a reader. This got into our codebase because someone copied it from this early code, and decided they didn't need the modification to the help. But as you can see from that code, the purpose of including it is adjust the default help for the parameter.