Skip to content
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

added Builder.copy() #904

Merged
merged 1 commit into from
May 14, 2024
Merged

added Builder.copy() #904

merged 1 commit into from
May 14, 2024

Conversation

zilto
Copy link
Collaborator

@zilto zilto commented May 13, 2024

This is useful in the Jupyter Magic V2, plugins and other scenarios where we want users to be able to pass a Builder() and another function be responsible for completing the Driver creation.

Changes

  • add Builder().copy() as currently implemented in the Jupyter magics V2 PR

How I tested this

  • check the copy points to a different object
  • check the copy and original have equal attributes

Notes

Currently, the copied Builder() holds references to the original Builder attributes. AFAIK, this means the original Builder() can be garbage collected, but not its attribute (assuming they're useful to the new Builder copy. An odd edge case would be:

  1. create Builder foo with stateful adapters
  2. copy Builder foo to create Builder bar
  3. create Driver foo and Driver bar from both.
  4. both Driver seemingly share an adapter with the same state, which could be odd for CachingAdapter, ExperimentTracker, and others given they store metadata as they progress through the DAG

@zilto zilto requested review from skrawcz and elijahbenizzy May 13, 2024 23:34
@zilto zilto added the enhancement New feature or request label May 13, 2024
@zilto zilto merged commit 2c1eb9a into main May 14, 2024
23 checks passed
@zilto zilto deleted the feat/builder-copy branch May 14, 2024 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants