Raise error for groups that decorate non-operation functions

[kidrahahjo]

Description

Throw an error when a group decorates a non-operation function
Example

from flow import FlowProject

group = FlowProject.make_group("foo")

@group
def op1(job): # This function is not an operation function because FlowProject.operation decorator is not being used
    pass

if __name__ == "__main__":
    FlowProject()

Motivation and Context

Fix #538

Types of Changes

• Documentation update
• Bug fix
• New feature
• Breaking change¹

¹The change breaks (or has the potential to break) existing functionality.

Checklist:

• I am familiar with the Contributing Guidelines.
• I agree with the terms of the Contributor Agreement.
• My name is on the list of contributors.
• My code follows the code style guideline of this project.
• The changes introduced by this pull request are covered by existing or newly introduced tests.

If necessary:

• I have updated the API documentation as part of the package doc-strings.
• I have created a separate pull request to update the framework documentation on signac-docs and linked it here.
• I have updated the changelog.

[codecov]

Codecov Report

Merging #544 (774c799) into master (d628c39) will decrease coverage by 0.09%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #544      +/-   ##
==========================================
- Coverage   78.36%   78.27%   -0.10%     
==========================================
  Files          31       31              
  Lines        3087     3088       +1     
  Branches      598      598              
==========================================
- Hits         2419     2417       -2     
- Misses        521      526       +5     
+ Partials      147      145       -2     

Impacted Files	Coverage Δ
flow/project.py	79.89% <100.00%> (-0.19%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d628c39...774c799. Read the comment docs.

[bdice]

I revised the error message text and pushed a change. I had a question about the __name__ attribute.

Also we should consider whether invalid FlowProject definitions should raise a ValueError or a RuntimeError, I put a message in Slack about that and copied it below.

FlowProject definition errors: Should we prefer RuntimeError or ValueError? Current behavior:

ValueErrors:

• Attempting to put more than one @FlowProject.operation decorator on the same function
• Attempting to define another operation with the same name as as existing operation
• Registering a condition function as an operation
• Registering an operation as a condition
• Defining a single function as an operation for two projects A and B, where B inherits from A
• Using FlowProject.pre.after with a function isn't an operation
• Creating an operation with the same name as an existing group
• Creating a group with the same name as an existing operation
• (Proposed in this PR): Registering a group on a function that is not an operation

RuntimeErrors:

• Using @cmd before @with_job
• Using @aggregator and @with_job on the same operation (in either order)

[bdice]

This code currently fails when used with custom operation names.

from flow import FlowProject


class Project(FlowProject):
    pass


group = Project.make_group("group")


@group
@Project.operation("custom_operation_name")
def print_job(job):
    print(job)


if __name__ == "__main__":
    Project().main()

Raises error:

ValueError: Cannot add the function 'print_job' to a FlowGroup because it is not registered as an operation. Add the `@FlowProject.operation` decorator to register the operation.

Edit: I thought for a while and experimented with some code but I don't see a solution for this. If we assume the function is registered as an operation first (@FlowProject.operation("name") is below the group decorator) then we could inject an attribute like _flow_operation_name and read that instead of __name__ when the group decorator is applied. But if the decorators are reversed and the group decorator is first, I don't see how we could identify the custom operation name...

[vyasr]

...
Edit: I thought for a while and experimented with some code but I don't see a solution for this. If we assume the function is registered as an operation first (@FlowProject.operation("name") is below the group decorator) then we could inject an attribute like _flow_operation_name and read that instead of __name__ when the group decorator is applied. But if the decorators are reversed and the group decorator is first, I don't see how we could identify the custom operation name...

My suggestion on the corresponding issue here was also predicated on enforcing an ordering for the decorators. At the time I said that this might not be desirable, but I think we should consider requiring that the operation decorator be applied before the group decorator. We already have a precedent (although in a significantly less visible context) for the with_job and cmd decorators. The more validation logic we push to "run-time" rather than "definition-time", the less performant we're likely to be. Moreover, you could argue that conceptually it makes sense to have decorators nested in a consistent order to make the code readable. Too much flexibility (especially when it doesn't actually enable anything new) can also be a bad thing and it obviously hamstrings us internally in some ways.

[kidrahahjo]

I don't think it's too much of an effort to add a check for the case which @bdice mentioned. The solution which immediately comes to my mind requires storing the __name__ property of the operation functions as an instance variable. But as @vyasr said, too much flexibility is not always great and the positioning of the decorators conceptually makes sense too. @bdice let me know what you think?

[bdice]

@kidrahahjo @vyasr I think it is fine to require an order for the decorators.

[vyasr]

I think the path forward then is to require an order for the decorators and then just look up the names in _OPERATION_FUNCTIONS.

[bdice]

I'm taking a look at this PR and will try to finalize it tonight. @b-butler said he and @kidrahahjo are planning to release 0.16 tomorrow.

[bdice]

@b-butler @kidrahahjo I forgot to handle decorator ordering when I worked on this last night, and have retracted my approval. There is still some additional work needed for that, so I suggest that we bump this to a later version. 👍

[b-butler]

@bdice sounds good. I will change the milestone for this PR and associated issue then. 😃

[b-butler]

This is looking good. A few more changes and this should be ready.

This could also be considered a breaking change, so what are thoughts on merging this without a deprecation cycle? The error message does give the fix which helps mitigate any pain in switching so I am fine with merging.

[vyasr]

I'm going to look into the outstanding status error here and see if I can help resolve it so that we can get this merged.

[vyasr]

@bdice I don't think there's anything wrong here. If you look at the test project definition the old version of op1 was actually not decorated with the operation decorator. I assume that this was an oversight, but as a result the outputs of the status commands now contain an extra operation which throws off alignment (e.g. in the eligible_jobs_max_lines=2 case that was the first failure that I saw) and total lines output (in the unroll=False case that is failing now after I merged in master). We should be safe to update the reference data for status tests.

[vyasr]

This is looking good. A few more changes and this should be ready.

This could also be considered a breaking change, so what are thoughts on merging this without a deprecation cycle? The error message does give the fix which helps mitigate any pain in switching so I am fine with merging.

Yeah, I'm fine just merging this. To an extent this is almost like a bug fix, it's resolving some undesirable edge cases in behavior.

[vyasr]

@b-butler let me know if you're happy with the way that I addressed your comments, I had to make some adaptations to deal with inherited operations between projects and to avoid conflicts.

[b-butler]

I have one suggestion and I will approve.

[b-butler]

👍