Save output model to output_dir #1430
Conversation
| @@ -321,17 +321,56 @@ def test_run_no_search(self, mock_local_system_init, tmp_path): | |||
| assert output_node.model_config == onnx_model_config | |||
| assert expected_metrics == output_node.metrics.value | |||
|
|
|||
| output_model_dir = output_dir / "output_model" | |||
There was a problem hiding this comment.
please update the tests in this too
Olive/test/unit_test/cli/test_cli.py
Line 116 in acc8763
There was a problem hiding this comment.
Line 412 in acc8763
There was a problem hiding this comment.
curious why this test can pass
There was a problem hiding this comment.
the test passes since base.py was not updated and we just mock the run output. we might have to update one of the cli tests to not do the mock.
| @@ -422,3 +429,30 @@ def _plot_pareto_frontier(self, ranks=None, save_path=None, is_show=True, save_f | |||
|
|
|||
| if is_show: | |||
| fig.show() | |||
|
|
|||
|
|
|||
| def get_best_candidate_node( | |||
There was a problem hiding this comment.
could you add some comments here to explain the logic?
|
|
||
| B. Multiple pass flows: | ||
| output_dir/output_model/{pass_flow}/...: Same as A but for each pass flow | ||
| output_dir/{pass_flow}/...: Same as A but for each pass flow |
There was a problem hiding this comment.
since there is only one save_model call, I don't think this structure is correct?
Please update the search and no search output model structure in this docstring after running and validating all scenarios.
There was a problem hiding this comment.
This case is same for both search and no search. Engine can still get a output_footprints and only one model will be saved to output folder. These are intermediate folders created by the engine. I can double check this and paste the folder structure later here.
There was a problem hiding this comment.
They have metrics file in each pass flow pass folder. Though I don't think those files are needed so we can keep the output folder clean. We can remove those intermediate folders later if needed.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -284,7 +282,7 @@ def test_quantize_command(mock_repo_exists, mock_tempdir, mock_run, algorithm_na | |||
| mock_tempdir.return_value = tmpdir.resolve() | |||
| mock_run.return_value = {} | |||
|
|
|||
| workflow_output_dir = tmpdir / "output_model" / algorithm_name | |||
| workflow_output_dir = tmpdir / algorithm_name | |||
There was a problem hiding this comment.
I don't think the algorithm name exists anymore? This was for the behavior where each pass flow saved it's own output model
There was a problem hiding this comment.
@xiaoyu-work please take a look at this too
| @@ -422,9 +429,6 @@ def run_no_search( | |||
| json.dump(signal.to_json(), f, indent=4) | |||
| logger.info("Saved evaluation results of output model to %s", results_path) | |||
|
|
|||
| self.cache.save_model(model_id=model_ids[-1], output_dir=flow_output_dir, overwrite=True) | |||
There was a problem hiding this comment.
is this is not present, then I don't think this pass_flow nesting is present?
not sure if we should keep it or not
There was a problem hiding this comment.
See my previous, removing this means it does not save pass_flow models to this path, but we still have results_path = flow_output_dir / "metrics.json" in this path. My expectation is we could remove pass_flow path also.
Describe your changes
Save output model to output_dir
Checklist before requesting a review
lintrunner -a(Optional) Issue link