Repository navigation
feat: Improve inference CLI and update tests - #166
Conversation
| f"must be symbolic links, not copies. Up to first 10: {names}" | ||
| ) | ||
|
|
||
| CLASSIFICATION_MINI_TRAIN_SAMPLES_PER_CLASS = 100 |
There was a problem hiding this comment.
I would keep these two class constants (lines 86 and 87) above the definition of the validate_generated_split method (like we had before this change)
| agent_id=target_agent_id, | ||
| workspace_dir=str(target_workspace_dir), | ||
| ) | ||
| save_config(target_config) |
There was a problem hiding this comment.
Is this save necessary? The config file would be saved regardless at the beginning of the (forked) run, in initialize_run (line 84 of run_agent.py). Could we leave target_config in memory for environment restoration and let initialization persist the final config?
Unrelated to your changes: I would rename the save_config method to something that communicates about saving to file (e.g. write_config_file, or save_config_to_file)
There was a problem hiding this comment.
Config is loaded from the filesystem through the run - we need to update it with the new agent id, otherwise it will still have the old runs agent id. Currently this doesnt have any bugs linked to it afaik, but there could be a time where that discrepancy introduces a new bug.
Renaming was implemented.
| return None | ||
|
|
||
| def create_model(self, model_name: str, config: Config) -> OpenAIModel: | ||
| def create_model(self, model_name: str, config: Config) -> Model: |
There was a problem hiding this comment.
As providers can now return a generic Model type, should this method return a generic Model and not a OpenAIModel (line 109)? As well the docstring now is mentioning OpenAI sdk friendly
There was a problem hiding this comment.
The Model is an abstract class. We should keep the default OpenAI model return here since its the most widely compatible one.
There was a problem hiding this comment.
Makes sense. I was trying to point out the inconsistency between code and docstring, your change in the dostring is satisfactory from my end
There was a problem hiding this comment.
Could we retain the environment-export regression scenarios covering Conda overwriting pip packages, differing Conda/pip package names, and base-environment package leakage? These tests existed because those interactions are difficult to predict. The replacement mixed-environment test doesn’t cover all of them. Rewriting them through the production export/recreation interfaces makes sense, but I’d preserve the scenarios.
In particular I am referring to:
test_graphviz_pip_conda_name_mismatch_survives_recreation,
test_graphviz_conda_only_survives_recreation,
test_pip_before_conda_order_survives_recreation,
test_export_captures_actual_version_after_conda_overwrites_pip,
test_export_excludes_phantom_packages_from_base_env
Let me know what you think.
There was a problem hiding this comment.
Implemented these and added some reasonable parametrizable tests that could be easily expanded in the future if we find failing cases.
Skipped test_export_excludes_phantom_packages_from_base_env since i dont think it covers a realistic regression.
| default=Path(Config.BEST_ITERATION_SNAPSHOT_DIRNAME), | ||
| help="Iteration directory relative to --agent-dir", | ||
| ) | ||
| parser.add_argument( |
There was a problem hiding this comment.
Could we put --all-iterations and --artifacts-dir in a parser.add_mutually_exclusive_group()? This would reject the combination during
parsing with a standard CLI error.
If you are applying this change, the test test_inference_requires_one_iteration_when_artifacts_are_supplied in tests/unit/test_inference_cli.py would need to expect SystemExit with code 2 during parsing instead of ValueError during execution.
| validate_docker_gpu_access(image) | ||
|
|
||
| container_name = f"agentomics-tests-{uuid4().hex}" | ||
| docker_arguments = ["--name", container_name, "--pull", "never"] |
There was a problem hiding this comment.
Among the arguments for the created containers, we should pass also the proxy env variables (if set).
| environment = { | ||
| key: value for key, value in os.environ.items() | ||
| if key in { | ||
| "PATH", "DOCKER_HOST", "DOCKER_CONTEXT", "DOCKER_CONFIG", |
There was a problem hiding this comment.
In here the proxy vars should be forwarded as end-to-end workflows recreate conda environments which require network access.
| parser.add_argument("--cpu-only", action="store_true", help="Disable GPU access and GPU tests") | ||
| arguments = parser.parse_args(argv) | ||
|
|
||
| image = arguments.image or build_development_image() |
There was a problem hiding this comment.
The development image-build path needs the proxy support from PR #164. Could we incorporate that change once merged? In here build_development_image() is called, so no separate build-proxy implementation should be needed here.
There was a problem hiding this comment.
This is automatically solved by merging with main that includes PR 164 since the proxy env variables are automatically fetched from the .env file
(will rebase)
andrea-gariboldi
left a comment
There was a problem hiding this comment.
Good job on this. I left some minor comments.
I can confirm I run the tests on a machine with GPU, and the tests are passing.
|
Small additional note: I was able to run the tests on our server after patching mysefl the proxy env vars forwarding. |
Extract env creation in tests Add a graphviz recreation test Add conda package recreation test Parametrize env recreation tests with package infos Add pip-conda-pip order env recreation test Add a pip-conda order env recreation test add pip and pip-conda packages env recreation tests
… the environment exports
andrea-gariboldi
left a comment
There was a problem hiding this comment.
Comments from round 1 have been addressed. I added a very small cleanup of an import.
This PR
I wasnt able to run GPU tests, so please run them on a GPU machine during the review