Valay/refactor on eddie's PR #3
Draft
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
- Unfinished - Ran black
- no notification system in place. - re-orged stuff into a module
- Failure of workers triggers failure of control. - Failure of control results in failure of workers. - Added more comments. - control tasks have heartbeat thread that write for a long enough period of time.
….deepspeed` - ensures separation of concern and also helps when debugging user code failures as there are distinct .
savingoyal
reviewed
Mar 6, 2024
| self.environment = environment | ||
| self.flow_datastore = flow_datastore | ||
|
|
||
| self.is_batch = False |
There was a problem hiding this comment.
where is is_batch and is_k8s used?
There was a problem hiding this comment.
I think it is not. Will remove.
savingoyal
reviewed
Mar 6, 2024
|
|
||
| for deco in decos: | ||
| if deco.name in ["resources", "kubernetes", "batch"]: | ||
| compute_deco_attrs = compute_resource_attributes( |
There was a problem hiding this comment.
is this function needed here? we are planning on removing this from metaflow OSS soon.
savingoyal
reviewed
Mar 6, 2024
| DEEPSPEED_SUFFIX = "mf.deepspeed_datastore" | ||
|
|
||
|
|
||
| HOSTFILE_IP_KEY = "hostfile_ips" |
There was a problem hiding this comment.
these are available in constants.py ?
savingoyal
reviewed
Mar 6, 2024
| TaskFailedException, | ||
| ) | ||
|
|
||
| DEEPSPEED_SUFFIX = "mf.deepspeed_datastore" |
There was a problem hiding this comment.
minor nit - just mf.deepspeed perhaps
savingoyal
reviewed
Mar 6, 2024
| class DeepspeedExecutor: | ||
| """ | ||
| Instances of the DeepspeedExecutor class are responsible for launching the Deepspeed command. There is one per Metaflow @step annotated with @deepspeed. | ||
| DeepspeedExecutor consumes a list of hosts aka nodes aka k8s pods aka metaflow task containers, and information about how many processes to run on each. |
There was a problem hiding this comment.
The executor is called within the user code. It represents current.deepspeed
- made the hello example work out of the box. - refactor on fixes.
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.