-
Notifications
You must be signed in to change notification settings - Fork 77
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add setup_environment
to truss
#1188
Add setup_environment
to truss
#1188
Conversation
@@ -298,6 +298,12 @@ def _run_docker(gpus: Optional[str] = None): | |||
f"src={str(LocalConfigHandler.bptr_data_resolution_dir_path())}", | |||
"target=/bptr", | |||
], | |||
[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for my own edification, what does this help accomplish?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we mimic the mounted configmap directory (which is /etc/b10_dynamic_config
on model pods) here so we can make changes to /etc/b10_dynamic_config/environment
in the integration tests to mimic patches to the environment
key in the dynamic_config
configmap
59a4aa9
to
3bae49a
Compare
|
||
# Skip polling if no setup_environment implementation provided | ||
if not self.model_descriptor.setup_environment: | ||
self._logger.info("No model.setup_environment definition provided") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to log here, if the user didn't provide a setup_environment method, I don't think this is the right way to communicate this to them.
On the other hand, if they did provide a method, it might be good to log something indicating that we will poll for changes to the env.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests look good, would you mind making this a context builder and testing on dev/staging? Ideally we can run truss-examples on it (I can walk you through how to do that)
@@ -96,6 +97,7 @@ pytest = "7.2.0" | |||
pytest-cov = "^3.0.0" | |||
types-PyYAML = "^6.0.12.12" | |||
types-setuptools = "^69.0.0.0" | |||
types-aiofiles = "^24.1.0.20240626" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use a less specific version here? (maybe "^24.1.0")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
going to see if i can update this as a follow-up, still running into poetry lock --no-update
hanging for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last comment, but this looks awesome, amazing work here!!
bffc63b
to
ba0e5fd
Compare
🚀 What
This PR adds a new
setup_environment
function that we will call when we notice a change to the environment a truss is running in. This environment refers to the release environment, ex:production
orstaging
. In order to notice these changes, we will need to watch for updates to a particularenvironment
file mounted on theb10_dynamic_config
directory.Going to add support for
setup_environment
for chainlets in a separate PR: #1192💻 How
on_startup
, we call a `setup_polling_for_environment_updates() function that will create the asyncio task that will run the file watcherModelWrapper._status
to beREADY
, indicating that theload()
was successfully completed, and then will start actually watching the/etc/b10_dynamic_config/environment
filedynamic_config_resolver
to access the contents of the/etc/b10_dynamic_config/environment
file and call the user-definedsetup_environment
function with the contents🔬 Testing
TODO:
setup_environment
fromload_impl
- this is to enable setting up the environment before user-definedload
is calledtruss/tests/test_model_inference.py
dynamic_config_resolver
Loom with e2e test: https://www.loom.com/share/cbe8e03a7d97434f99f973eaceb33c59?sid=ca51f099-fd45-4b39-a7bf-366d7d01356b