Conversation
4e7a050 to
5e33063
Compare
31f5251 to
c7b2292
Compare
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
… output format Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
…ith info cmd) Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
…lflow and wandb Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
…nel installation) Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
c7b2292 to
65f0925
Compare
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
pribalta
left a comment
There was a problem hiding this comment.
Power Review
Reviewed 30 files, left 21 suggestions: 2 errors, 13 warnings, 6 suggestions.
Dimensions Analyzed
| Dimension | 🔴 | 💡 | |
|---|---|---|---|
| anti_patterns | 0 | 4 | 1 |
| architecture | 1 | 3 | 1 |
| patterns | 1 | 1 | 1 |
| security-risks | 0 | 3 | 0 |
| testing-quality | 0 | 2 | 3 |
| Total | 2 | 13 | 6 |
A pull request for exporters refactor arrives,
Our review engine carefully contrives.
Still, only you can see the bigger view,
So weigh these findings before you approve.
Automated review by Power Review — findings are advisory.
packages/nemo-evaluator-launcher/src/nemo_evaluator_launcher/exporters/wandb.py
Show resolved
Hide resolved
packages/nemo-evaluator-launcher/src/nemo_evaluator_launcher/exporters/utils.py
Outdated
Show resolved
Hide resolved
packages/nemo-evaluator-launcher/tests/unit_tests/test_cli_export_extended.py
Outdated
Show resolved
Hide resolved
packages/nemo-evaluator-launcher/src/nemo_evaluator_launcher/exporters/utils.py
Show resolved
Hide resolved
packages/nemo-evaluator-launcher/src/nemo_evaluator_launcher/cli/info.py
Outdated
Show resolved
Hide resolved
| _, job_dir = prepare_local_job(jd, with_required=True, with_optional=True) | ||
| artifacts_dir = job_dir / "artifacts" | ||
| # Add required files | ||
| (artifacts_dir / "run_config.yml").write_text( |
There was a problem hiding this comment.
[Power Review] 💡 Identical artifact file setup code duplicated across all TestLocalExporterManualScenarios methods · 88% confidence
Every test in TestLocalExporterManualScenarios (test_successful_export_multiple_jobs, test_partial_failure_missing_artifacts, test_non_existing_job_id, test_remote_job_export_with_copy, test_json_format_updates_existing_file) copies the same two write_text calls verbatim for run_config.yml and results.yml. This is the Duplicated Test Setup anti-pattern: shared setup should be extracted into a fixture or helper to reduce maintenance burden and the risk of individual tests drifting out of sync.
💡 Suggestion: Extract the artifact-file creation into a local helper, e.g.:
def _write_standard_artifacts(artifacts_dir: Path) -> None:
(artifacts_dir / 'run_config.yml').write_text(
'framework_name: test-harness\nconfig:\n type: test_task\n'
'target:\n api_endpoint:\n model_id: test-model\n'
)
(artifacts_dir / 'results.yml').write_text(
'results:\n tasks:\n test_task:\n metrics:\n'
' accuracy:\n scores:\n accuracy:\n value: 0.9\nconfig: {}\n'
)Or add a pytest fixture to conftest.py.
| inv = "test001" | ||
| artifacts_dir = tmp_path / inv / f"{inv}.0" / "artifacts" | ||
| artifacts_dir.mkdir(parents=True) | ||
|
|
There was a problem hiding this comment.
[Power Review] 💡 Identical artifact directory setup duplicated across all MLflow exporter test methods · 85% confidence
Five tests (test_export_job_ok, test_export_with_skip_existing, test_export_with_update_existing, test_log_config_params_flattens_config, test_log_config_params_with_max_depth) each copy the same three-step pattern: mkdir the artifacts directory, write run_config.yml, write results.yml with identical content. This is textbook Duplicated Test Setup. Extracting it into a fixture would shorten each test and ensure the artifact format is consistent.
💡 Suggestion: Add a pytest fixture (possibly extending the existing prepare_local_job fixture in conftest.py) that creates and returns a pre-populated artifacts directory. Each test can then request this fixture instead of repeating the setup inline.
packages/nemo-evaluator-launcher/src/nemo_evaluator_launcher/exporters/mlflow.py
Outdated
Show resolved
Hide resolved
packages/nemo-evaluator-launcher/src/nemo_evaluator_launcher/exporters/base.py
Outdated
Show resolved
Hide resolved
packages/nemo-evaluator-launcher/tests/unit_tests/exporters/test_gsheets_exporter.py
Show resolved
Hide resolved
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
Signed-off-by: Marta Stepniewska-Dziubinska <martas@nvidia.com>
No description provided.