Unbounded foreach #450
Closed
Unbounded foreach #450
Conversation
This file contains 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
* As denoted by plugins/test_unbounded_foreach/* this outlines a template for creating (unbounded) foreach which requires a control task that baby-sits the execution of a foreach while delegating the actual execution of the foreach task possibly to a separate service. * Concrete use-cases include a hyperparameter tuning service, or a different service implementing a map-reduce paradigm but with a specialized purpose. TODO: * Enable tests in local mode after bugs in datastore (py2, py3 incompat); local metadata service (w.r.t task registration) are fixed. * CliChecker might have to be updated to allow accessing a run due to the presence of local metadata provider.
* Add ability to access local metadata through CLIChecker for accessing run objects for UBF tests. * Rewrite the expectation in nested_unbounded_foreach to only consider UBF tasks since `dump` isn't aware of control vs ubf tasks.
romain-intel
commented
Mar 19, 2021
| # This allows decorators to have access to the CLI options instead of relying | ||
| # (solely) on the click context. | ||
| # TODO(crk): Fold `dict_to_cli_options` as a private method of this `CLIArgs` | ||
| # once all other callers of `step` [titus, meson etc.] are unified. |
There was a problem hiding this comment.
Comment not relevant to OSS or at least needs to be modded to remove irrelevant references.
| @@ -154,6 +155,11 @@ def kill(ctx, run_id, user, my_runs): | |||
| @click.option("--shared_memory", help="Shared Memory requirement for AWS Batch.") | |||
| @click.option("--max_swap", help="Max Swap requirement for AWS Batch.") | |||
| @click.option("--swappiness", help="Swappiness requirement for AWS Batch.") | |||
| @click.option('--ubf-context', | |||
| default=None, | |||
| type=click.Choice([None, UBF_CONTROL, UBF_TASK]), | |||
There was a problem hiding this comment.
Seems to be a discrepancy between the 'none' option in cli.py and None here. Is this intended?
| @@ -337,14 +439,17 @@ def _queue_tasks(self, finished_tasks): | |||
| self._is_cloned[task.path] = task.is_cloned | |||
|
|
|||
| # CHECK: ensure that runtime transitions match with | |||
| # statically inferred transitions | |||
| # statically inferred transitions. Make an exception for control | |||
There was a problem hiding this comment.
Nit: indentation issue.
| # This signals the runtime that the task doesn't want any | ||
| # retries indifferent to other decorator opinions. | ||
| # NOTE: We don't statically disallow specifying `@retry` in | ||
| # combination with other decorators (e.g. `@archer`) only to |
There was a problem hiding this comment.
Internal comment needs rewording.
| @@ -799,7 +933,8 @@ def _launch(self): | |||
| for deco in self.task.decos: | |||
| deco.runtime_step_cli(args, | |||
| self.task.retries, | |||
| self.task.user_code_retries) | |||
| self.task.user_code_retries, | |||
| self.task.ubf_context) | |||
There was a problem hiding this comment.
Nit: Indentation
| @@ -290,8 +290,7 @@ def dict_to_cli_options(params): | |||
| if k == 'decospecs': | |||
| k = 'with' | |||
| k = k.replace('_', '-') | |||
| if not isinstance(v, tuple): | |||
| v = [v] | |||
| v = v if isinstance(v, list) or isinstance(v, tuple) else [v] | |||
| for value in v: | |||
| yield '--%s' % k | |||
| if not isinstance(value, bool): | |||
There was a problem hiding this comment.
Internal PR had a to_unicode here. Is this no longer relevant?
|
@seeravikiran: could you make the small changes suggested and then we can merge if that's ok. |
|
@romain-intel Is this PR linked to datastore changes or can I go ahead and test this PR on top of master? |
savingoyal
requested changes
Apr 20, 2021
| @@ -61,7 +63,8 @@ def _merge_lists(base, overrides, attr): | |||
| RetryDecorator, | |||
| BatchDecorator, | |||
| StepFunctionsInternalDecorator, | |||
| CondaStepDecorator], ext_plugins.STEP_DECORATORS, 'name') | |||
| CondaStepDecorator, | |||
| TestUnboundedForeachDecorator], ext_plugins.STEP_DECORATORS, 'name') | |||
There was a problem hiding this comment.
I don't think we should expose this decorator right now.
|
I think you can port this PR on top of master. I don't think it has a strong dependency (but things will change a bit). |
Merged
|
Replaced by #554 |
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.
No description provided.