Lightening studio orchestrator #2931
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add Documentation and Community
|
github-actions
bot
added
internal
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
|
NLP template updates in |
|
|
||
| ### Lightning AI UI | ||
|
|
||
| Lightning AI provides its own UI where you can monitor and manage your running applications, including the pipelines orchestrated by ZenML. |
|
|
||
| ```python | ||
| lightning_settings = LightningOrchestratorSettings( | ||
| machine_type="gpu", |
There was a problem hiding this comment.
give the list of options currently available?
| custom_validation_function=_validate_remote_components, | ||
| ) | ||
|
|
||
| def _get_lightning_client( |
There was a problem hiding this comment.
This does not get a lightning client, it sets lighning env variables
src/zenml/utils/code_utils.py
Outdated
| with tempfile.NamedTemporaryFile( | ||
| mode="w+b", delete=False, suffix=".tar.gz" | ||
| ) as f: | ||
| code_archive.write_archive(f) | ||
|
|
||
| hash_ = hashlib.sha1() # nosec | ||
|
|
||
| f.seek(0) |
There was a problem hiding this comment.
Was this a bug with the hash computation before or why is this seek needed?
There was a problem hiding this comment.
it was suggested by claude auto-complete
There was a problem hiding this comment.
That doesn't mean you shouldn't think about it or verify that it makes sense. In this case, the seeking already happens at the end of code_archive.write_archive() which means it's not necessary here.
src/zenml/utils/code_utils.py
Outdated
|
|
||
| Returns: | ||
| The path where to archived code is uploaded. | ||
| A tuple containing the temporary file path and the computed hash. |
There was a problem hiding this comment.
The second item of this tuple is not the computed hash, but instead a filename. I think returning two filenames is not intuitive at all what they're supposed to be.
As far as I can see, this function is used below and in the lightning orchestrator, which actually doesn't even use the hash.
I would suggest we just use
with tempfile.NamedTemporaryFile(
mode="w+b", delete=False, suffix=".tar.gz"
) as f:
code_archive.write_archive(f)in the lightning orchestrator (and also in the function below), I don't think that's worth having in a separate helper function.
I still like your idea of extracting the hash computation though, maybe we can make this compute_file_hash(file) -> str?
There was a problem hiding this comment.
I think it's just doc string error later on i was also using the filename as second output
There was a problem hiding this comment.
Still I think it would be nicer to have a helper function that computes just the hash of a file instead of having both the archive writing and hash computation combined in one function.
If we had a util function to compute just the file hash, we have other places in the code where we could reuse it.
If you still think that we should not duplicate this code
with tempfile.NamedTemporaryFile(
mode="w+b", delete=False, suffix=".tar.gz"
) as f:
code_archive.write_archive(f)maybe we create a second helper function which is called write_archive_to_temporary_file(...) -> str which does this and returns the filename?
…ub.com/zenml-io/zenml into feature/lightening-studio-orchestrator
|
No dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No dependency changes detected in pull request |
…ub.com/zenml-io/zenml into feature/lightening-studio-orchestrator
|
Images automagically compressed by Calibre's image-actions ✨ Compression reduced images by 45.5%, saving 429.78 KB.
292 images did not require optimisation. Update required: Update image-actions configuration to the latest version before 1/1/21. See README for instructions. |
src/zenml/utils/code_utils.py
Outdated
| with tempfile.NamedTemporaryFile( | ||
| mode="w+b", delete=False, suffix=".tar.gz" | ||
| ) as f: | ||
| code_archive.write_archive(f) | ||
|
|
||
| hash_ = hashlib.sha1() # nosec | ||
|
|
||
| f.seek(0) |
There was a problem hiding this comment.
That doesn't mean you shouldn't think about it or verify that it makes sense. In this case, the seeking already happens at the end of code_archive.write_archive() which means it's not necessary here.
src/zenml/utils/code_utils.py
Outdated
|
|
||
| Returns: | ||
| The path where to archived code is uploaded. | ||
| A tuple containing the temporary file path and the computed hash. |
There was a problem hiding this comment.
Still I think it would be nicer to have a helper function that computes just the hash of a file instead of having both the archive writing and hash computation combined in one function.
If we had a util function to compute just the file hash, we have other places in the code where we could reuse it.
If you still think that we should not duplicate this code
with tempfile.NamedTemporaryFile(
mode="w+b", delete=False, suffix=".tar.gz"
) as f:
code_archive.write_archive(f)maybe we create a second helper function which is called write_archive_to_temporary_file(...) -> str which does this and returns the filename?
| DEPLOYMENT_ID_OPTION = "deployment_id" | ||
|
|
||
|
|
||
| class LightningOrchestratorEntrypointConfiguration: |
There was a problem hiding this comment.
Why is this not inheriting and specifying the deployment ID again?
| logger.info("Main studio started.") | ||
| logger.info("Uploading code to main studio the code path: %s", filename) | ||
| main_studio.upload_file( | ||
| "/teamspace/studios/this_studio/.lightning_studio/.studiorc", |
There was a problem hiding this comment.
I have not checked this, but there must be some overlap between this code and the code in the orchestrator (some commands that are run in the studio, ...). We should refactor this to use some common util functions in the future.
Describe changes
I implemented/fixed _ to achieve _.
Pre-requisites
Please ensure you have done the following:
developand the open PR is targetingdevelop. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes