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.
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.
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
Support to configure executor for benchmark #634
Support to configure executor for benchmark #634
Changes from 2 commits
cb4edf3
5032cc1
116a509
4c291bf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think executor should be part of the config? If this affects parallelism and results it may be better to include it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought this a little bit. I took it out for 2 consideration:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we would need to persist the configuration in the case of the parallelization assessment? I seems to me it would be better to. Maybe that can be another PR, since it would require supporting configuration of the executor through dict arguments instead of simply an instantiated object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like here: https://github.com/Epistimio/orion/pull/634/files/cb4edf3667330ad6863cfde698f7591b91a281ed..5032cc1fa5c1323d65f6aa0c405530ac9816f617#diff-850efe1c00bd8d6203e80177ccbf6bcc12058cb1d15c8d56b8902728379bf94cR313
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For parallelization assessment, yes, I think we need to pass-in executor name and configure instead of an instance for persistence, this is because it is part of the benchmark and should not change when we launch same benchmark at different nodes.