Feature/sagemaker compiler #662
Conversation
GeorgesLorre
requested review from
RobbeSneyders,
PhilippeMoussalli and
mrchtr
src/fondant/pipeline/compiler.py
Outdated
| msg, | ||
| ) | ||
|
|
||
| def build_command( |
There was a problem hiding this comment.
So this could be reused by other compilers and be abstracted more
There was a problem hiding this comment.
would make it an internal function and rename it to get_build_command()
|
|
||
| steps: t.List[t.Any] = [] | ||
|
|
||
| with tempfile.TemporaryDirectory(dir=os.getcwd()) as tmpdirname: |
There was a problem hiding this comment.
the scripts are uploaded to s3 by sagemaker once we compile the pipeline that is why we save the files to a temp folder that only gets deleted at the end.
|
So the coverage dropping is quite painful but the coverage overall is pretty bad for |
src/fondant/pipeline/compiler.py
Outdated
| output_path: the path where to save the Kubeflow pipeline spec. | ||
| instance_type: the instance type to use for the processing steps | ||
| (see: https://aws.amazon.com/ec2/instance-types/ for options). | ||
| role_arn: the role arn to use for the processing steps, |
There was a problem hiding this comment.
| role_arn: the role arn to use for the processing steps, | |
| role_arn: the Amazon Resource Name role to use for the processing steps, |
There was a problem hiding this comment.
wasn't very clear what "arn" is
src/fondant/pipeline/compiler.py
Outdated
| def compile( | ||
| self, | ||
| pipeline: Pipeline, | ||
| output_path: str, | ||
| instance_type: str = "ml.t3.medium", | ||
| role_arn: t.Optional[str] = None, |
There was a problem hiding this comment.
| def compile( | |
| self, | |
| pipeline: Pipeline, | |
| output_path: str, | |
| instance_type: str = "ml.t3.medium", | |
| role_arn: t.Optional[str] = None, | |
| def compile( | |
| self, | |
| pipeline: Pipeline, | |
| *, | |
| output_path: str, | |
| instance_type: str = "ml.t3.medium", | |
| role_arn: t.Optional[str] = None, |
| ) | ||
|
|
||
| if not role_arn: | ||
| role_arn = self.sagemaker.get_execution_role() |
There was a problem hiding this comment.
Can you add a comment on what this does exactly?
tests/test_compiler.py
Outdated
| ] | ||
|
|
||
| # with dependencies | ||
| dependencies = ["component_2"] |
There was a problem hiding this comment.
small nitpick but would name it component_1. Right now it looks like the component is dependent on itself
src/fondant/pipeline/compiler.py
Outdated
| component_name: str, | ||
| command: t.List[str], | ||
| directory: str, | ||
| ): |
There was a problem hiding this comment.
Could you add a return type?
src/fondant/pipeline/compiler.py
Outdated
| ) | ||
| depends_on = [steps[-1]] if component["dependencies"] else [] | ||
|
|
||
| script = self.generate_component_script( |
There was a problem hiding this comment.
Could you rename script to script_path. At first glance it seems like the function is returning the script string
src/fondant/pipeline/compiler.py
Outdated
| msg, | ||
| ) | ||
|
|
||
| def build_command( |
There was a problem hiding this comment.
would make it an internal function and rename it to get_build_command()
src/fondant/pipeline/runner.py
Outdated
| @@ -65,7 +65,7 @@ def run( | |||
| experiment = self.client.get_experiment(experiment_name=experiment_name) | |||
| except ValueError: | |||
| logger.info( | |||
| f"Defined experiment '{experiment_name}' not found. Creating new experiment" | |||
| f"defined experiment '{experiment_name}' not found. creating new experiment" | |||
There was a problem hiding this comment.
I think the previous formatting was ok
tests/test_compiler.py
Outdated
| compiler = SagemakerCompiler() | ||
| command = ["echo", "hello world"] | ||
| with tmp_path_factory.mktemp("temp") as fn: | ||
| script = compiler.generate_component_script("component_1", command, fn) |
There was a problem hiding this comment.
Same here with the path
| script = compiler.generate_component_script("component_1", command, fn) | |
| script_path = compiler.generate_component_script("component_1", command, fn) |
GeorgesLorre
force-pushed
the
feature/sagemaker-compiler
branch
from
53f9e72 to
7018ce2
Compare
resolves: #661