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

#186 - Add FunctionParameter model and reduce schema parsing #203

Merged

Conversation

scott-taubman
Copy link
Contributor

@scott-taubman scott-taubman commented Feb 7, 2023

Closes #186

This switches the Function model to having its parameter definitions stored in a new model FunctionParameter, rather than through the pre-generated schema. The reasons for doing this are:

  • It makes Function and Workflow consistent in their parameter handling
  • It makes interacting with the Function parameters (find parameters of type X) much easier, since wasn't any tooling for parsing and interacting with the pydantic generated schema.
  • It makes us must less tied to the pydantic library and its schema format, since we have the parameter data in a format that we can easily feed into any library of our choosing.

The main changes that are happening here:

  • A new FunctionParameter model now exists. It and WorkflowParameter share a common base, so those two models and the base model now live in core/models/parameter.py.
  • The builder has been updated to create (and delete) FunctionParameter instances rather than generating a pydantic schema.
  • The TaskParameterForm now uses the info from the FunctionParameter instances to construct the form, rather than relying on the schema.
  • There is now a core/utils/parameter.py (not to be confused with the now deleted parameters.py) that is the home of all of the logic that deals with pydantic. Generating the schema and validating parameters against that schema all happen here. This file is also now the home of the single source of parameter type definitions. The various places that had defined their own parameter types now just import it from here.

Other related changes:

  • The code that was in core/utils/serialization.py was no longer needed after these changes so that file is now gone. A much simpler helper method that just finds the json parameters from the database is included in core/utils/parameter.py and used internally by t he validated_parameters function there.
  • Many of the unit tests needed to be update to replace the function schema in various fixtures with FunctionParameter instances.

Test Instructions

  • Some new unit tests were written to confirm that the validation on WorkflowRun and the FunctionParameter removal during the build process work as expected.
  • Make sure that package builds still work, functions are still taskable, and field validation in the UI still works. The behavior should be the same as before in all cases.

@scott-taubman scott-taubman force-pushed the 186-function-parameter-model branch 2 times, most recently from 116a2b0 to 45dde74 Compare February 7, 2023 17:54
("json", "JSON"),
]
RETURN_TYPES = PARAMETER_TYPES[:]
RETURN_TYPE_CHOICES = PARAMETER_TYPE_CHOICES[:]
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'm not entirely clear on why this is a separate thing from PARAMETER_TYPE_CHOICES, but that's how it was before and I decided to just keep it (though I renamed it for consistency).

_load_dockerfile_template(dockerfile, workdir)
# Need to validate the potentially new function schema, but
# don't save it until the build has finished.
functions, function_parameters = _create_functions_from_definition(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This got bumped outside of the try block since:

  • The exceptions being handled didn't seem like they could be raised by the lines here.
  • The linter was rightfully complaining later on that technically functions could be unset.

package.image_name = image_name
package.save()

_delete_removed_function_parameters(function_parameters, package)
_deactivate_removed_functions(function_definitions, package)
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 realized this deactivate_removed_functions call was in the wrong spot before, so I fixed that here.

functionary/core/models/function.py Show resolved Hide resolved
@scott-taubman scott-taubman marked this pull request as ready for review February 7, 2023 18:43
"boolean": (BooleanField, None),
"date": (DateField, HTMLDateInput),
"date-time": (
PARAMETER_TYPE.INTEGER: (IntegerField, None),
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 know one of the complaints has been that there are multiple mappings to go update when we add a parameter type. I don't know that we can get it down to only 1 place, since in a case like this it's a mapping that's hyper specific to the TaskParameterForm. I think having the constants defined at least makes it a little less error prone.

@kvig
Copy link
Contributor

kvig commented Feb 8, 2023

I was testing and accidentally deleted part of a function and left the parameters section. The package.yaml ended up with two parameter sections:

      # (required) Parameters that the function takes
      parameters:
        - name: a
          type: integer
          default: 1
          required: true
        - name: b
          type: integer
          default: 1
          required: true

      # (required) Parameters that the function takes
      parameters: []

I don't know if this was a feature before the refactor, but if we can catch this misconfiguration that'd be good thing. I'd error on multiple parameter definitions.

@scott-taubman
Copy link
Contributor Author

The package.yaml ended up with two parameter sections...if we can catch this misconfiguration that'd be good thing

We unfortunately cannot detect this particular error, since the library we're using (pyyaml) merges duplicate keys rather than throwing an error. This goes against the yaml spec, but it seems like they've never reached consensus on how to fix it. See discussions here and here.

functionary/core/models/function.py Show resolved Hide resolved
functionary/core/models/function.py Show resolved Hide resolved
functionary/core/utils/tasking.py Outdated Show resolved Hide resolved
functionary/core/models/parameter.py Show resolved Hide resolved
@xeonneon xeonneon merged commit 4089291 into beer-garden:main Feb 9, 2023
@scott-taubman scott-taubman deleted the 186-function-parameter-model branch March 7, 2023 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revisit Function Schema Storage and Validation
3 participants