Skip to content

fix: resolve TypeError when passing multi-events via config_expr in @trigger#3081

Open
Satyamgupta2365 wants to merge 5 commits into
Netflix:masterfrom
Satyamgupta2365:fix/sort-lazy-plugin-commands
Open

fix: resolve TypeError when passing multi-events via config_expr in @trigger#3081
Satyamgupta2365 wants to merge 5 commits into
Netflix:masterfrom
Satyamgupta2365:fix/sort-lazy-plugin-commands

Conversation

@Satyamgupta2365
Copy link
Copy Markdown

Description

This PR fixes a bug where using config_expr to define multiple events in the @trigger decorator resulted in a TypeError: ConfigValue is immutable when trying to set event parameters.

Context

When @trigger(events=config_expr(...)) is used, the configuration values are returned as ConfigValue objects. These objects are immutable and act as wrappers around the underlying dictionary to prevent accidental modifications.

The TriggerDecorator.process_event method, however, expects a mutable object because it attempts to set event["parameters"] directly. This leads to the following traceback:

File "metaflow/plugins/events_decorator.py", line 122, in process_event
  event["parameters"] = self.process_parameters(...)
File "metaflow/user_configs/config_parameters.py", line 190, in __setitem__
  raise TypeError("ConfigValue is immutable")
TypeError: ConfigValue is immutable

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 3, 2026

Greptile Summary

This PR fixes a TypeError: ConfigValue is immutable that occurred when using config_expr to define multiple events in the @trigger decorator. When config_expr returns ConfigValue objects (immutable dict subclasses), the process_event method's direct assignment event["parameters"] = ... failed. The fix adds event = dict(event) on line 107 to create a shallow mutable copy before any mutation, which is sufficient for all current code paths since process_parameters does not mutate its parameters argument in place.

  • metaflow/plugins/events_decorator.py: One-line fix that shallow-copies the incoming event dict to a plain mutable dict, resolving the immutability conflict with ConfigValue objects produced by config_expr.
  • metaflow/cli_components/utils.py: Unrelated change that wraps the list_commands return value in sorted() for deterministic alphabetical CLI command ordering; benign and correct on its own.

Confidence Score: 5/5

Safe to merge — the bug fix is correct and the only other change is a benign sort.

The core fix (dict shallow copy) correctly resolves the TypeError for all current code paths. No new P0 or P1 issues were identified beyond concerns already addressed in prior review threads. The unrelated sorting change is harmless. All remaining observations are P2 or below.

No files require special attention.

Important Files Changed

Filename Overview
metaflow/plugins/events_decorator.py Adds shallow dict copy in process_event to allow mutation of immutable ConfigValue from config_expr, correctly fixing the TypeError
metaflow/cli_components/utils.py Adds sorted() to list_commands return for deterministic alphabetical ordering; unrelated to main bug fix but benign

Greploops — Automatically fix all review issues by running /greploops in Claude Code. It iterates: fix, push, re-review, repeat until 5/5 confidence.
Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal.

Reviews (3): Last reviewed commit: "Merge branch 'master' into fix/sort-lazy..." | Re-trigger Greptile

if is_stringish(event):
return {"name": str(event)}
elif isinstance(event, dict):
event = dict(event)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Shallow copy may not be sufficient for deeply-nested ConfigValue parameters

dict(event) creates a shallow copy — the top-level dict becomes mutable, but any nested ConfigValue objects (e.g. the value of event["parameters"]) remain as ConfigValue instances. In process_parameters, isinstance(parameters, dict) will be True for a ConfigValue (since it inherits from dict), and iterating over it works fine, so the immediate bug is resolved. However, for better defensive programming, consider using ConfigValue.to_dict() (which performs a full recursive conversion to plain dicts) rather than a shallow dict() copy:

elif isinstance(event, dict):
    # Deep copy to allow mutation (handles immutable ConfigValue from config_expr)
    event = event.to_dict() if hasattr(event, "to_dict") else dict(event)

This is a suggestion rather than a blocking issue — the shallow copy is sufficient for all current code paths.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

subgroup = self._lazy_load(source_name, source)
base.extend(subgroup.list_commands(ctx))
return base
return sorted(base)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Unrelated change bundled with the @trigger bug fix

This change (sorting the list_commands output in LazyPluginCommandCollection) is unrelated to the TypeError: ConfigValue is immutable bug described in the PR. It has no connection to the @trigger decorator or config_expr.

While the change itself is benign and reasonable (producing deterministic, alphabetically-ordered CLI command listings), bundling it with an unrelated bug fix makes the PR harder to review and bisect. Consider splitting this into a separate PR or adding a note in the PR description explaining why this change is included.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

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

Successfully merging this pull request may close these issues.

1 participant