modal-labs / modal-client

Python client library for Modal
https://modal.com/docs
Apache License 2.0
271 stars 35 forks source link

Rebuild image on changes to static class variables #1959

Closed TheQuantumFractal closed 3 months ago

TheQuantumFractal commented 3 months ago

Describe your changes

Addresses MOD-2989

This code change allows for static class variables that are accessed by the build function to be tracked for rebuilds. In particular, it determines the static class variables of the parent class if one is defined. It then traverses the code of the function to see which class attributes are accessed. The set of accessed static class variables is then tracked alongside the globals in the build_function definition.

Also added tests (tests need Mock Client Servicer to only build an image on new requests for ImageGetOrCreate)

Backward/forward compatibility checks --- Check these boxes or delete any item (or this section) if not relevant for this PR. - [-] Client+Server: this change is compatible with old servers **NOTE: This change will force image rebuilds for all classes with static class variables** - [x] Client forward compatibility: this change ensures client can accept data intended for later versions of itself Note on protobuf: protobuf message changes in one place may have impact to multiple entities (client, server, worker, database). See points above. ---

Changelog

mwaskom commented 3 months ago

Not a comment on the implementation, but IMO, this might produce a net decrease in the coherence of the client behavior. I think it's especially a little weird to rebuild the image on changes to static class variables — but not on changes to class initialization parameters — especially since init params are one of the main reasons to use the class Function pattern.

I can't see the Slack discussion that's linked from the Linear ticket though; @aksh-at do you remember the original context here?