Skip to content

Commit

Permalink
Change @context_params behavior to only pass explicit arguments. (#1136)
Browse files Browse the repository at this point in the history
Previously it would attempt to pass missing arguments as part of kwargs, but this caused issues because Ray tried to interpret them.
  • Loading branch information
bsowell authored Jan 28, 2025
1 parent 3c8fba3 commit fae9db7
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 25 deletions.
16 changes: 5 additions & 11 deletions lib/sycamore/sycamore/context.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,19 +117,13 @@ def wrapper(*args, **kwargs):
candidate_kwargs.pop(param, None)

"""
If the function doesn't accept arbitrary kwargs, we don't want to use candidate_kwargs that aren't in
the function signature.
Remove candidate kwargs that aren't in the function signature.
"""
new_kwargs = {}
accepts_kwargs = any(param.kind == param.VAR_KEYWORD for param in sig.parameters.values())

if accepts_kwargs:
new_kwargs = candidate_kwargs
else:
for param in signature[len(args) :]: # arguments that haven't been passed as positional args
candidate_val = candidate_kwargs.get(param)
if candidate_val:
new_kwargs[param] = candidate_val
for param in signature[len(args) :]: # arguments that haven't been passed as positional args
candidate_val = candidate_kwargs.get(param)
if candidate_val:
new_kwargs[param] = candidate_val

"""
Put in user provided kwargs (either through decorator param or function call)
Expand Down
14 changes: 0 additions & 14 deletions lib/sycamore/sycamore/tests/unit/test_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,17 +110,3 @@ def test_positional_args_and_context_args():

# Combine positional and kwarg
assert "a b" == two_positional_args_method_with_kwargs("a", some_other_arg="b", context=context)


def test_positional_args_and_context_args_f_with_kwargs():
context = Context(
params={"default": {"some_other_arg": "Aryn2", "some_unrelated_arg": "ArynZ"}}, exec_mode=ExecMode.LOCAL
)
# Pickup 'some_other_arg' from context
assert "a Aryn2" == two_positional_args_method_with_kwargs(some_function_arg="a", context=context)

# Should ignore context vars because of kwargs
assert "a b" == two_positional_args_method_with_kwargs(some_function_arg="a", some_other_arg="b", context=context)

# Combine positional and kwarg
assert "a b" == two_positional_args_method_with_kwargs("a", some_other_arg="b", context=context)

0 comments on commit fae9db7

Please sign in to comment.