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

MNT Improve handling of custom-defined results from TaskParameter models. #51

Open
gadorlhiac opened this issue Sep 11, 2024 · 1 comment
Labels
invalid This doesn't seem right

Comments

@gadorlhiac
Copy link
Collaborator

gadorlhiac commented Sep 11, 2024

Description

TaskParameter models can define the result of the associated Task to be an arbitrary combination of parameters (or other features) by using the result_from_params field stored in the TaskParameters.Config configuration class.

We should improve the way this is done.

Issue

Currently, the documented mechanism for storing information in this field is to use a custom validator to update the class variable (TaskParameters.Config.result_from_params).

The class variable needs to be converted to an instance variable of the parameter model during initialization of a Task instance. This is because no instance of the Config class is stored in the parameters model (only the class definition). Updates to the class variables within the Config class do not survive pickling, since by choice, pickle will not save code or class variables. This means when the validated model is pickled and sent to the Executor, the changes to result_from_params introduced by the custom validator would be lost upon unpickling.

To avoid this, the above conversion to an instance variable is done. This requires, in turn, skipping over the new instance variable when either ThirdPartyTask's or the Executor are inspecting the model. E.g. the ThirdPartyTask must make sure not to pass result_from_params as a command-line argument.

Ideally, this complicated mechanism would be simplified.

Documentation must be updated to reflect any new mechanism.

Note: This may necessitate changes to the run_directory mechanism as well. However, run_directory does not suffer from this problem since that information is only used by the Task, so it is fine if lost during pickling.

@gadorlhiac gadorlhiac added the invalid This doesn't seem right label Sep 11, 2024
@gadorlhiac
Copy link
Collaborator Author

This may be easier to address with pydantic v2. (I.e. without major changes to other code).
#63 will provide support for v2 but will not remove support for v1. Perhaps after v1 support is removed updates to address this issue can be made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

1 participant