Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update ExperimentManager interface and implement plugin methods for StandardExperimentManager #147

Merged
merged 7 commits into from
Jan 4, 2022
Merged

Update ExperimentManager interface and implement plugin methods for StandardExperimentManager #147

merged 7 commits into from
Jan 4, 2022

Conversation

romanwozniak
Copy link
Contributor

@romanwozniak romanwozniak commented Dec 20, 2021 •
edited
Loading

Context:

This PR is a continuation of #143, where we introduced the general structure for RPC-based experiment engine plugins. This PR updates ExperimentManager interface definition to make it compatible with the plugin implementation and extends RPC plugin (client/server) to support methods of the StandardExperimentManager interface.

Changes:

  • ValidateExperimentConfig/GetExperimentRunnerConfig methods now standardized and moved under ExperimentManager. Previously, signatures of these methods were different for StandardExperimentManager and CustomExperimentManager, which was tricky to support in RPC plugin, because rpcClient should implement methods of both standard/custom ExperimentManager (to support communication with both types).
  • rpc_server/rpc_client were extended to support methods of the StandardExperimentManager interface
  • The rest is simply changes to existing unit tests and new tests to cover added code parts

@romanwozniak romanwozniak marked this pull request as draft December 20, 2021 15:40
@romanwozniak romanwozniak changed the title Draft: Update experiment manager interface and implement plugin methods for standard experiment manager Update ExperimentManager interface and implement plugin methods for StandardExperimentManager Dec 21, 2021
@romanwozniak romanwozniak requested a review from a team December 21, 2021 07:00
@romanwozniak romanwozniak self-assigned this Dec 21, 2021
@romanwozniak romanwozniak marked this pull request as ready for review December 21, 2021 07:01
krithika369
krithika369 approved these changes Jan 4, 2022
View reviewed changes
Copy link
Collaborator

@krithika369 krithika369 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The experiment manager interface looks much more concise now. Thanks for the changes. LGTM!

api/turing/validation/validator.go Outdated Show resolved Hide resolved
engines/experiment/plugin/rpc/manager/rpc_client.go Outdated Show resolved Hide resolved
engines/experiment/plugin/rpc/manager/rpc_client.go Outdated Show resolved Hide resolved
@romanwozniak romanwozniak merged commit 1f22856 into caraml-dev:main Jan 4, 2022
@romanwozniak romanwozniak deleted the update_experiment_manager_interface branch January 4, 2022 05:37
@romanwozniak romanwozniak mentioned this pull request Jan 13, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krithika369 krithika369 krithika369 approved these changes

Assignees

@romanwozniak romanwozniak

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@romanwozniak @krithika369