Create convenience decorator as_step_fn for building StepSpecs#5157
Create convenience decorator as_step_fn for building StepSpecs#5157
as_step_fn for building StepSpecs#5157Conversation
|
Claude finished @wmoss's task in 4m 55s —— View job Code review
Two high-signal issues found. Inline comments posted:
Recommendation on the PR question ("is this useful enough to add?"): yes, once the decorator is fixed to be a pure curry (reject |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 536aac8c88
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
This allows for decorating a function that you want to use in a `StepSpec` and avoiding the `lambda op: ...` boilerplate.
536aac8 to
45d6bc4
Compare
|
@claude please review again, I fixed the bug that caused all your original concerns. |
|
Claude finished @wmoss's task in 1m 44s —— View job Re-review after fix
Verdict: fix looks correct; both original concerns are resolved. What changed
def wrapper(*args, **kwargs):
if "output_path" not in kwargs:
def inner(output_path):
return fn(*args, output_path=output_path, **kwargs)
return inner
return fn(*args, **kwargs)That matches the docstring — a normal call with Call-site auditWalked every caller of
Since Minor nits (non-blocking)
Happy with merging as-is otherwise. |
|
@wmoss this is nice, but it introduces magic. I find magic is not good for humans, nor agents. Are we sure this kind of magic is worth it? |
There was a problem hiding this comment.
@wmoss this is nice, but it introduces magic. I find magic is not good for humans, nor agents. Are we sure this kind of magic is worth it?
Yeah, this is basically the question I am asking by putting up the PR. Does the usefulness out-weight the magic? (I also just added a slightly less magical, but also less ergonomic version in a comment).
FWIW, decorators are a fairly normal form of Python magic. For example, click, which we use for clis, uses them extensively.
I can certainly see the arguments in favor of either choice though.
| return fn(*args, output_path=output_path, **kwargs) | ||
|
|
||
| return inner | ||
| return fn(*args, **kwargs) |
There was a problem hiding this comment.
This function could be simplified to,
@wraps(fn)
def wrapper(*args: Any, **kwargs: Any) -> Any:
def inner(output_path: str) -> Any:
return fn(*args, output_path=output_path, **kwargs)
return inner
which makes it less weird / magical IMO, but does mean that you can no longer call this function like a normal function and so tests would need to change to look like,
result = dedup_exact_paragraph(
input_paths=fox_corpus["test_dir"],
max_parallelism=4,
)(output_path=fox_corpus["output_dir"])
There was a problem hiding this comment.
yea, I'm not sure if I like the less weird/magical version. we probably should go either full magic or little-magic, IMHO I like little-magic, especially in the world where the agents write the code (and I'm just a reader). wdyt?
There was a problem hiding this comment.
That's a fair point. I'm certainly reading more code as I'm trying to learn how things work than I will be in the future.
This allows for decorating a function that you want to use in a
StepSpecand avoiding thelambda op: ...boilerplate.Curious if people think this is useful enough to add.