feat(piplines, components)[AutoML]: add new param eval_metric#114
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR exposes eval_metric as a pipeline/component input for AutoGluon tabular and timeseries training. Tabular component accepts eval_metric: str = "" and conditionally defaults to task-type metric (regression → "r2", else "accuracy"); it validates non-blank and membership in METRICS and returns the resolved metric. Timeseries component accepts eval_metric: str = "MASE", removes the internal fallback, validates membership in AVAILABLE_METRICS, and propagates the input into predictor construction and returned model_config. Both pipelines surface and wire the parameter; unit tests and test configs are added or extended to cover validation, forwarding, default resolution, and artifact metadata. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
components/training/automl/autogluon_models_training/component.py (2)
178-179: 💤 Low valueConsider adding whitespace validation for defensive consistency.
The defaulting logic uses a truthiness check that catches empty string and
None, but not whitespace-only strings like" ". Other string parameters in this component (e.g.,label_columnat line 98-99) validate with.strip().While AutoGluon's
TabularPredictorwill reject invalid metric names anyway, adding a.strip()check would be more defensive and consistent:if not eval_metric or not eval_metric.strip(): eval_metric = "r2" if task_type == "regression" else "accuracy"This matches the validation pattern used by the downstream
leaderboard_evaluationcomponent (which checkseval_metric.strip()per the context snippet).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/training/automl/autogluon_models_training/component.py` around lines 178 - 179, The eval_metric defaulting uses a truthiness check that misses whitespace-only strings; update the conditional around eval_metric in the AutoGluon training component so it treats empty or whitespace-only values as unset (e.g., use eval_metric.strip() in the condition), then default to "r2" for task_type == "regression" or "accuracy" otherwise; locate the logic that sets eval_metric (the eval_metric variable assignment in the autogluon_models_training component) and change the check to reject whitespace-only strings consistent with other validations like label_column.strip().
30-30: 💤 Low valueType hint inconsistency with test expectations.
The signature declares
eval_metric: str = "", but the unit test attest_component_unit.py:1390explicitly passeseval_metric=None. While this works at runtime (the truthiness check at line 178 handles both""andNone), it violates the type contract.For consistency with similar optional parameters like
positive_class: Optional[str] = None(line 29), consider either:
- Update the signature to
eval_metric: Optional[str] = ""to explicitly acceptNone, or- Keep the signature as-is and update the test to pass
eval_metric=""instead ofNone.Option 2 is cleaner since the empty string default already triggers the desired defaulting behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/training/automl/autogluon_models_training/component.py` at line 30, The parameter eval_metric is declared as eval_metric: str = "" but tests pass None; to keep the type contract, update the unit test to pass eval_metric="" (empty string) instead of None so the existing defaulting/truthiness logic still applies; do not change the function signature—leave eval_metric as str = "" to match other optional string params like positive_class and ensure the test uses an empty string.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@components/training/automl/autogluon_timeseries_models_training/component.py`:
- Around line 129-130: The current guard only checks that eval_metric is a
non-empty string but doesn't ensure it's one of AutoGluon’s supported metrics;
import autogluon.timeseries.metrics.AVAILABLE_METRICS and add a membership check
after the existing isinstance/strip guard (where eval_metric is validated) to
raise a TypeError if eval_metric not in AVAILABLE_METRICS, including a message
listing or referencing AVAILABLE_METRICS to show valid options; keep the
exception type as TypeError and update any tests or usage that assume the
earlier behavior.
---
Nitpick comments:
In `@components/training/automl/autogluon_models_training/component.py`:
- Around line 178-179: The eval_metric defaulting uses a truthiness check that
misses whitespace-only strings; update the conditional around eval_metric in the
AutoGluon training component so it treats empty or whitespace-only values as
unset (e.g., use eval_metric.strip() in the condition), then default to "r2" for
task_type == "regression" or "accuracy" otherwise; locate the logic that sets
eval_metric (the eval_metric variable assignment in the
autogluon_models_training component) and change the check to reject
whitespace-only strings consistent with other validations like
label_column.strip().
- Line 30: The parameter eval_metric is declared as eval_metric: str = "" but
tests pass None; to keep the type contract, update the unit test to pass
eval_metric="" (empty string) instead of None so the existing
defaulting/truthiness logic still applies; do not change the function
signature—leave eval_metric as str = "" to match other optional string params
like positive_class and ensure the test uses an empty string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c9257135-ad04-4817-a36c-028e7d084c9d
📒 Files selected for processing (14)
components/training/automl/autogluon_models_training/README.mdcomponents/training/automl/autogluon_models_training/component.pycomponents/training/automl/autogluon_models_training/tests/test_component_unit.pycomponents/training/automl/autogluon_timeseries_models_training/README.mdcomponents/training/automl/autogluon_timeseries_models_training/component.pycomponents/training/automl/autogluon_timeseries_models_training/tests/test_component_unit.pypipelines/training/automl/autogluon_tabular_training_pipeline/README.mdpipelines/training/automl/autogluon_tabular_training_pipeline/pipeline.pypipelines/training/automl/autogluon_tabular_training_pipeline/tests/test_configs.jsonpipelines/training/automl/autogluon_tabular_training_pipeline/tests/test_pipeline_unit.pypipelines/training/automl/autogluon_timeseries_training_pipeline/README.mdpipelines/training/automl/autogluon_timeseries_training_pipeline/pipeline.pypipelines/training/automl/autogluon_timeseries_training_pipeline/tests/test_configs.jsonpipelines/training/automl/autogluon_timeseries_training_pipeline/tests/test_pipeline_unit.py
Mateusz-Switala
force-pushed
the
feat-automl-add-new-param-eval-metric
branch
from
9e0c72b to
d8cc0d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@components/training/automl/autogluon_models_training/tests/test_component_unit.py`:
- Around line 1400-1526: The tests only exercise eval_metric=None but the
pipeline default is an empty string, so add explicit test coverage for
eval_metric=="" (the pipeline sentinel) so the component behaves the same as
None; update or add tests alongside
test_eval_metric_none_regression_resolves_to_r2,
test_eval_metric_none_binary_resolves_to_accuracy and
test_eval_metric_none_multiclass_resolves_to_accuracy to call
autogluon_models_training.python_func with eval_metric="" and assert the same
resolved metrics and constructor arg (check
mock_predictor_class.call_args[1]["eval_metric"] and result.eval_metric), or
alternatively make the component signature accept Optional[str] end-to-end and
update those tests to pass "" as the real default sentinel.
In
`@pipelines/training/automl/autogluon_timeseries_training_pipeline/tests/test_pipeline_unit.py`:
- Around line 113-121: The test reads the compiled YAML into the bytes variable
content and then performs string containment checks, causing a TypeError; fix by
decoding content to an ASCII string once after reading (e.g., replace using
content.decode("ascii") -> assign back to content_str) and use that decoded
string for the subsequent assertions and decode-based error handling in the
try/except block around tmp_path reading/unlinking; update references to content
in the assertions ("componentInputParameter: eval_metric" and
"outputParameterKey: eval_metric") to use the decoded string variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7ec26a22-045f-4080-9d45-d13a1c8ffd5e
📒 Files selected for processing (14)
components/training/automl/autogluon_models_training/README.mdcomponents/training/automl/autogluon_models_training/component.pycomponents/training/automl/autogluon_models_training/tests/test_component_unit.pycomponents/training/automl/autogluon_timeseries_models_training/README.mdcomponents/training/automl/autogluon_timeseries_models_training/component.pycomponents/training/automl/autogluon_timeseries_models_training/tests/test_component_unit.pypipelines/training/automl/autogluon_tabular_training_pipeline/README.mdpipelines/training/automl/autogluon_tabular_training_pipeline/pipeline.pypipelines/training/automl/autogluon_tabular_training_pipeline/tests/test_configs.jsonpipelines/training/automl/autogluon_tabular_training_pipeline/tests/test_pipeline_unit.pypipelines/training/automl/autogluon_timeseries_training_pipeline/README.mdpipelines/training/automl/autogluon_timeseries_training_pipeline/pipeline.pypipelines/training/automl/autogluon_timeseries_training_pipeline/tests/test_configs.jsonpipelines/training/automl/autogluon_timeseries_training_pipeline/tests/test_pipeline_unit.py
✅ Files skipped from review due to trivial changes (4)
- components/training/automl/autogluon_models_training/README.md
- pipelines/training/automl/autogluon_tabular_training_pipeline/README.md
- components/training/automl/autogluon_timeseries_models_training/README.md
- pipelines/training/automl/autogluon_timeseries_training_pipeline/README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- pipelines/training/automl/autogluon_timeseries_training_pipeline/tests/test_configs.json
- pipelines/training/automl/autogluon_tabular_training_pipeline/pipeline.py
- pipelines/training/automl/autogluon_timeseries_training_pipeline/pipeline.py
- components/training/automl/autogluon_timeseries_models_training/component.py
- components/training/automl/autogluon_models_training/component.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@components/training/automl/autogluon_models_training/tests/test_component_unit.py`:
- Around line 1400-1526: The tests only exercise eval_metric=None but the
pipeline default is an empty string, so add explicit test coverage for
eval_metric=="" (the pipeline sentinel) so the component behaves the same as
None; update or add tests alongside
test_eval_metric_none_regression_resolves_to_r2,
test_eval_metric_none_binary_resolves_to_accuracy and
test_eval_metric_none_multiclass_resolves_to_accuracy to call
autogluon_models_training.python_func with eval_metric="" and assert the same
resolved metrics and constructor arg (check
mock_predictor_class.call_args[1]["eval_metric"] and result.eval_metric), or
alternatively make the component signature accept Optional[str] end-to-end and
update those tests to pass "" as the real default sentinel.
In
`@pipelines/training/automl/autogluon_timeseries_training_pipeline/tests/test_pipeline_unit.py`:
- Around line 113-121: The test reads the compiled YAML into the bytes variable
content and then performs string containment checks, causing a TypeError; fix by
decoding content to an ASCII string once after reading (e.g., replace using
content.decode("ascii") -> assign back to content_str) and use that decoded
string for the subsequent assertions and decode-based error handling in the
try/except block around tmp_path reading/unlinking; update references to content
in the assertions ("componentInputParameter: eval_metric" and
"outputParameterKey: eval_metric") to use the decoded string variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 7ec26a22-045f-4080-9d45-d13a1c8ffd5e
📒 Files selected for processing (14)
components/training/automl/autogluon_models_training/README.mdcomponents/training/automl/autogluon_models_training/component.pycomponents/training/automl/autogluon_models_training/tests/test_component_unit.pycomponents/training/automl/autogluon_timeseries_models_training/README.mdcomponents/training/automl/autogluon_timeseries_models_training/component.pycomponents/training/automl/autogluon_timeseries_models_training/tests/test_component_unit.pypipelines/training/automl/autogluon_tabular_training_pipeline/README.mdpipelines/training/automl/autogluon_tabular_training_pipeline/pipeline.pypipelines/training/automl/autogluon_tabular_training_pipeline/tests/test_configs.jsonpipelines/training/automl/autogluon_tabular_training_pipeline/tests/test_pipeline_unit.pypipelines/training/automl/autogluon_timeseries_training_pipeline/README.mdpipelines/training/automl/autogluon_timeseries_training_pipeline/pipeline.pypipelines/training/automl/autogluon_timeseries_training_pipeline/tests/test_configs.jsonpipelines/training/automl/autogluon_timeseries_training_pipeline/tests/test_pipeline_unit.py
✅ Files skipped from review due to trivial changes (4)
- components/training/automl/autogluon_models_training/README.md
- pipelines/training/automl/autogluon_tabular_training_pipeline/README.md
- components/training/automl/autogluon_timeseries_models_training/README.md
- pipelines/training/automl/autogluon_timeseries_training_pipeline/README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- pipelines/training/automl/autogluon_timeseries_training_pipeline/tests/test_configs.json
- pipelines/training/automl/autogluon_tabular_training_pipeline/pipeline.py
- pipelines/training/automl/autogluon_timeseries_training_pipeline/pipeline.py
- components/training/automl/autogluon_timeseries_models_training/component.py
- components/training/automl/autogluon_models_training/component.py
🛑 Comments failed to post (2)
components/training/automl/autogluon_models_training/tests/test_component_unit.py (1)
1400-1526:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winCover the actual pipeline default sentinel.
These fallback tests only exercise
eval_metric=None, but the pipeline contract now defaultseval_metricto"". A component that resolvesNonebut rejects""would still pass this suite and then fail when the pipeline omitseval_metric. Add explicit cases foreval_metric=""on regression and classification, or make the component contractOptional[str]end-to-end.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/training/automl/autogluon_models_training/tests/test_component_unit.py` around lines 1400 - 1526, The tests only exercise eval_metric=None but the pipeline default is an empty string, so add explicit test coverage for eval_metric=="" (the pipeline sentinel) so the component behaves the same as None; update or add tests alongside test_eval_metric_none_regression_resolves_to_r2, test_eval_metric_none_binary_resolves_to_accuracy and test_eval_metric_none_multiclass_resolves_to_accuracy to call autogluon_models_training.python_func with eval_metric="" and assert the same resolved metrics and constructor arg (check mock_predictor_class.call_args[1]["eval_metric"] and result.eval_metric), or alternatively make the component signature accept Optional[str] end-to-end and update those tests to pass "" as the real default sentinel.pipelines/training/automl/autogluon_timeseries_training_pipeline/tests/test_pipeline_unit.py (1)
113-121:
⚠️ Potential issue | 🟠 Major | ⚡ Quick win🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '103,121p' pipelines/training/automl/autogluon_timeseries_training_pipeline/tests/test_pipeline_unit.py python - <<'PY' content = b"componentInputParameter: eval_metric" try: "componentInputParameter: eval_metric" in content except TypeError as exc: print(f"{type(exc).__name__}: {exc}") PYRepository: opendatahub-io/pipelines-components
Length of output: 1020
Fix failing ASCII-only test: decode YAML to
strbeforestrcontainment checks
contentisbytes, but the test asserts using string literals (e.g.,"componentInputParameter: eval_metric" in content), which raisesTypeErrorand prevents the wiring assertions from running. Decode once to ASCII text and assert on that decoded string.Patch
- content = Path(tmp_path).read_bytes() try: - content.decode("ascii") + content = Path(tmp_path).read_text(encoding="ascii") except UnicodeDecodeError as exc: pytest.fail(f"Compiled pipeline YAML must be ASCII-only: {exc}") finally: Path(tmp_path).unlink(missing_ok=True) assert "componentInputParameter: eval_metric" in content assert "outputParameterKey: eval_metric" in content📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.try: content = Path(tmp_path).read_text(encoding="ascii") except UnicodeDecodeError as exc: pytest.fail(f"Compiled pipeline YAML must be ASCII-only: {exc}") finally: Path(tmp_path).unlink(missing_ok=True) assert "componentInputParameter: eval_metric" in content assert "outputParameterKey: eval_metric" in content🧰 Tools
🪛 GitHub Actions: Run Tests and Validate Example Pipelines for Updated Components / 0_targeted-tests.txt
[error] 120-120: Test failed in test_compiled_pipeline_yaml_is_ascii_only: TypeError: a bytes-like object is required, not 'str' (assertion 'componentInputParameter: eval_metric' in content where content is bytes).
🪛 GitHub Actions: Run Tests and Validate Example Pipelines for Updated Components / targeted-tests
[error] 120-120: Test failed with TypeError in test_compiled_pipeline_yaml_is_ascii_only: 'a bytes-like object is required, not str' (assertion comparing a bytes content variable to a string).
[error] 104-121: Assertion failure in test_compiled_pipeline_yaml_is_ascii_only expecting "componentInputParameter: eval_metric" in compiled pipeline YAML, but the test crashes due to bytes-vs-str mismatch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pipelines/training/automl/autogluon_timeseries_training_pipeline/tests/test_pipeline_unit.py` around lines 113 - 121, The test reads the compiled YAML into the bytes variable content and then performs string containment checks, causing a TypeError; fix by decoding content to an ASCII string once after reading (e.g., replace using content.decode("ascii") -> assign back to content_str) and use that decoded string for the subsequent assertions and decode-based error handling in the try/except block around tmp_path reading/unlinking; update references to content in the assertions ("componentInputParameter: eval_metric" and "outputParameterKey: eval_metric") to use the decoded string variable.
Signed-off-by: Mateusz Switala <mswitala@redhat.com> Assisted-by: Claude Code
Signed-off-by: Mateusz Switala <mswitala@redhat.com> Assisted-by: Claude Code
Signed-off-by: Mateusz Switala <mswitala@redhat.com> Assisted-by: Claude Code
Signed-off-by: Mateusz Switala <mswitala@redhat.com> Assisted-by: Claude Code
Signed-off-by: Mateusz Switala <mswitala@redhat.com> Assisted-by: Claude Code
Signed-off-by: Mateusz Switala <mswitala@redhat.com> Assisted-by: Claude Code
Mateusz-Switala
force-pushed
the
feat-automl-add-new-param-eval-metric
branch
from
e84264d to
53c8038
Compare
|
@Mateusz-Switala Please rebase - let's merge it beginning of week Jun 8th, so UI team can pick up the changes. |
Signed-off-by: Mateusz Switala <mswitala@redhat.com>
|
@dryszka The branch has been already rebased. The review can be conducted. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: DorotaDR The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
Assisted-by: Claude Code
Description of your changes:
Adds an eval_metric parameter to both the tabular and timeseries AutoML training components and their corresponding pipelines, allowing callers to control the metric used for model ranking without forking the pipeline.
Tabular (autogluon_models_training + pipeline)
and returned directly (previously read from predictor.eval_metric after fit).
Timeseries (autogluon_timeseries_models_training + pipeline)
stored in model_config, and returned as a component output.
Documentation
README inputs tables for both components and both pipelines updated with the new eval_metric parameter.
Checklist:
Pre-Submission Checklist
Learn more about the pull request title convention used in this repository.
Additional Checklist Items for New or Updated Components/Pipelines
metadata.yamlincludes freshlastVerifiedtimestampare present and complete
snake_casenaming conventionSummary by CodeRabbit
New Features
Documentation
Tests