Extend Storage tests to multiple backends #986
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit 8a07c58.
This reverts commit 65e7f08.
…e-to-support-fractional-time
…e-to-support-fractional-time
for more information, see https://pre-commit.ci
…e-to-support-fractional-time
This reverts commit 45ad11f.
This reverts commit d77a35127c83366f0d53af664371a6ea9798a3f7.
This reverts commit f250e61.
This reverts commit d6617ba.
1. Allow restoring an Experiment by ID for parallel workers without needing to re-evaluate the git info. 2. Allow recovering the absolute root env rather than mistakenly re-resolving it from the root of the repo multiple times (which results in an error).
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Pull Request Overview
This PR extends the storage tests to support multiple backend types while refactoring and updating various SSH and storage-related fixtures, configuration files, and schema migration support. Key changes include parameterizing the storage fixture (covering mem_storage, postgres_storage, and mysql_storage), refactoring SSH fixture dependencies (e.g. replacing ssh_test_server_hostname with docker_hostname), and updating the SQL schema definitions/migrations together with enhancements to storage experiment handling.
Reviewed Changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| mlos_bench/tests/services/remote/ssh/fixtures.py | Refactored fixture dependency by replacing use of ssh_test_server_hostname with docker_hostname and removing an unused import. |
| mlos_bench/tests/services/remote/ssh/conftest.py | Removed redundant fixture alias for ssh_test_server_hostname. |
| mlos_bench/tests/services/remote/ssh/init.py | Cleaned up unused imports and inline documentation in the SSH module. |
| mlos_bench/tests/environments/local/*.py | Updated timestamp formatting in telemetry tests and removed rounding of microseconds. |
| mlos_bench/tests/conftest.py & mlos_bench/tests/init.py | Added new docker_hostname fixture and redefined wait_docker_service_socket for improved cross-platform compatibility. |
| mlos_bench/storage/sql/* | Introduced a new _reset_schema testing helper, updated column definitions to use dynamic length values, and refined experiment property handling for git configuration. |
| mlos_bench/storage/sql/alembic/* | Added migration scripts and updated the env configuration to support fractional seconds in MySQL. |
| mlos_bench/storage/base_storage.py | Modified the constructor to handle cases when root_env_config is None and required git parameters are provided, and added new properties for relative and absolute configuration paths. |
| mlos_bench/config/storage/* | Updated storage configuration files for consistency with the new database naming. |
Comments suppressed due to low confidence (4)
mlos_bench/tests/init.py:15
- There appear to be multiple definitions of wait_docker_service_socket across different modules. Consider refactoring this helper into a common utility to avoid duplication and ensure consistency.
def wait_docker_service_socket(docker_services: DockerServices, hostname: str, port: int) -> None:
mlos_bench/tests/environments/local/local_env_telemetry_test.py:18
- The removal of microsecond rounding may introduce precision issues in tests that expect whole-second timestamps. Verify that this change does not affect downstream comparisons or expected outputs.
ts1 -= timedelta(microseconds=ts1.microsecond) # Round to a second
mlos_bench/storage/base_storage.py:202
- Changing the constructor behavior to require git_repo, git_commit, and rel_root_env_config when root_env_config is None is a breaking change. Ensure that callers or tests have been updated to pass these parameters accordingly.
if root_env_config is None:
mlos_bench/tests/conftest.py:45
- The docker_hostname fixture now returns "127.0.0.1" instead of "localhost" on non-Windows platforms. Verify that this change is intentional and consistent with how hostnames are resolved in other parts of the test suite.
return "127.0.0.1" # "localhost"
bpkroth
added
WIP
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request
Title
Extend Storage test to multiple backends.
Description
storagefixture into a parameterized one that allows testing all storage unit test methods viamem_storage(sqlite),postgres_storage, andmysql_storagewhenever docker is available.Type of Change
Testing
Additional Notes (optional)
Part of a series of changes. Should be merged after Fixup and refactor root_env_config resolution #985.
There is currently a bit of a delay in running the tests due to the additional startup cost. May need to address that too.