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

PyTask now fails unit tests due to strict argument checking #328

Closed
computron opened this issue Jun 11, 2019 · 2 comments
Closed

PyTask now fails unit tests due to strict argument checking #328

computron opened this issue Jun 11, 2019 · 2 comments
Assignees

Comments

@computron
Copy link
Member

We recently added strict parameter checking to FireTasks when a developer specified optional_params - see discussion here for context:

hackingmaterials/atomate#295

However, PyTask now fails to meet the criteria. A user can for example set auto_kwargs to True, which allows them to set whatever kwargs they feel like and this breaks the strict parameter checking.

We do need a fix for this soon. A few options:

  • Remove the list of optional_params from PyTask. This will also automatically skip parameter checking. The only downside here is that in order to see the list of optional parameters for PyTask, you need to read the docstring. Seems like the best solution to me.
  • Add an option to FireTaskBase to skip parameter checking. This seems clunky (yet another variable to know about!) and I'd discourage this
  • Remove the auto_kwargs option from PyTask. Downsides are that it breaks backward compatibility and also removes the ability to use that function for people that like it. Potential upside is that this parameter is probably confusing to most people anyway. I'd also discourage this solution, at the very least due to not wanting to break backward compatibility.
@computron
Copy link
Member Author

(the specific test that fails is:
fireworks.user_objects.firetasks.tests.test_script_task.PyTaskTest#test_task_auto_kwargs
)

@computron
Copy link
Member Author

I implemented a solution which was to rename optional_params --> other_params

@shyuep If you think this is the best solution feel free to close this issue

If you have a better proposal feel free to suggest

@shyuep shyuep closed this as completed Jun 11, 2019
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

No branches or pull requests

2 participants