add extract method to MetaflowCode #2246
Conversation
| this object is garbage collected. | ||
| """ | ||
| exclusions = [ | ||
| "metaflow/", |
There was a problem hiding this comment.
I think we have constants for this so we should use that. +1 to factoring it out as well.
There was a problem hiding this comment.
why would we need to factor this out? #2238 can directly invoke this method unless i am missing something else?
There was a problem hiding this comment.
Recommend waiting on #2269 which will make this easier.
There was a problem hiding this comment.
So, discussion was:
- release now
- later will be made easier by 2269
- we can add a comment about where condav2-1.cnd comes from (but yes, it should be included)
metaflow/client/core.py
Outdated
| return tmp | ||
|
|
||
| @property | ||
| def script(self) -> str: |
There was a problem hiding this comment.
maybe: script_name. That would leave us the ability to return the actual script (whcih we already extract and have) if we want to without name conflcit.
metaflow/client/core.py
Outdated
| The directory and its contents are automatically deleted when | ||
| this object is garbage collected. | ||
| """ | ||
| EXCLUSIONS = [ |
There was a problem hiding this comment.
Can this be made configurable or put in metaflow_config instead? We might have some more files at Netflix, that should be excluded.
There was a problem hiding this comment.
do you have an example? ideally it would be nicer to include exclusions neither in the code or the config but the code package itself
There was a problem hiding this comment.
We have the conda*.cnd files in the code package.
There was a problem hiding this comment.
is it conda.cnd or something else?
There was a problem hiding this comment.
It's condav2-1.cnd specifically - I don't see the harm in making it configurable with sane defaults like the ones we already have.
There was a problem hiding this comment.
i have currently hard coded it in - ideally if we can avoid adding more to configs that would be my preference since it can become quite tricky to keep track of different versions of metaflow with a singular config. a proper solution would be to add the non-user files to INFO and then exclude them.
| this object is garbage collected. | ||
| """ | ||
| exclusions = [ | ||
| "metaflow/", |
There was a problem hiding this comment.
Recommend waiting on #2269 which will make this easier.
| @@ -878,6 +879,72 @@ def tarball(self) -> tarfile.TarFile: | |||
| """ | |||
| return self._tar | |||
|
|
|||
| @property | |||
| def extract(self) -> TemporaryDirectory: | |||
There was a problem hiding this comment.
would rename to extract_user_code or something like that. We discussed a bit and we could do:
- one method called extract with a flag that determines WHAT (user code, environment, everything)
extract_user_codewhich would be this and then the tarball extraction would be the other one.
I think we were leaning towards the second solution and that could work but it feels a little less symetric.
I think we also discussed a slightly different UX than the temporary directory. I guess the question is how will users use this.
There was a problem hiding this comment.
Make it a function instead of a property.
There was a problem hiding this comment.
Final comment from me on this:
- ok with name
extract - make it a function
No description provided.