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

Unnecessary list of one element per iteration in loop in YAML checker? #477

Open
yousefmoazzam opened this issue Oct 11, 2024 · 0 comments
Labels
question Further information is requested refactor

Comments

@yousefmoazzam
Copy link
Collaborator

yousefmoazzam commented Oct 11, 2024

Specifically, this is about the check for that parameters in the config for a method are in the template for that method:

def check_parameter_names_are_known(conf: PipelineConfig) -> bool:
"""
Check if the parameter name of config methods exists in yaml template method parameters
"""
template_yaml_conf = _get_template_yaml_conf(conf)
for method_dict in conf:
yml_method_list = [
yml_method_dict
for yml_method_dict in template_yaml_conf
if (yml_method_dict["method"] == method_dict["method"])
and (yml_method_dict["module_path"] == method_dict["module_path"])
]
unknown_param_dict = [
p
for yml_method in yml_method_list
for p, v in method_dict["parameters"].items()
if p not in yml_method["parameters"].keys()
]
for p in unknown_param_dict:
_print_with_colour(
f"Parameter '{p}' in the '{method_dict['method']}' method is not valid."
)
return False
return True

For some basic info:

  • conf is a list of method configs parsed from the YAML pipeline file to a python list
  • template_yaml_conf is a list of templates associated with the methods in the pipeline, parsed to python list

It seems like in the loop over the method configs in the pipeline file, the YAML template config associated with that method is searched for, and put in the the yml_method_list variable, which is a list:

for method_dict in conf:
yml_method_list = [
yml_method_dict
for yml_method_dict in template_yaml_conf
if (yml_method_dict["method"] == method_dict["method"])
and (yml_method_dict["module_path"] == method_dict["module_path"])
]

This is a bit confusing, since I would have expected that the conf and template_yaml_conf lists are the same length, due to each method config having one and only one YAML template associated with it. There doesn't seem to be a need to "search" for the YAML template config associated with each method, I would have thought doing an iteration over both conf and template_yaml_conf something would accomplish this, like:

for method_config, template_config in zip(conf, template_yaml_config):
  # TODO: check params in `method_config` are compatible with `template_config`, maybe using sets or something

and there's maybe no need for yml_method_list (again, which is a list only ever containing one element)?

@yousefmoazzam yousefmoazzam added question Further information is requested refactor labels Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested refactor
Projects
None yet
Development

No branches or pull requests

1 participant