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

Forking/parallelism tracking #218

Closed
wants to merge 12 commits into from
Closed

Forking/parallelism tracking #218

wants to merge 12 commits into from

Conversation

elijahbenizzy
Copy link
Contributor

@elijahbenizzy elijahbenizzy commented May 28, 2024

Allows forking/parallelism. Part (1) of this: #52. In this PR we allow the user to do their own.

See #52

Changes

  • Adds the __context input for nodes to declare
  • Enables __context to be nullable

TODO:

  • Add spawning parent pointer to the application builder
  • Get the tracker to work with multiple apps (so we can use the same tracker).
  • Wire info through to hooks
  • Add it to data tracking model for local client
  • Show in UI
    • App list
    • Step list
  • Add tests for tracking
  • Add documentation

How I tested this

Notes

Checklist

  • PR has an informative and human-readable title (this will be pulled into the release notes)
  • Changes are limited to a single goal (no scope creep)
  • Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future TODOs are captured in comments
  • Project documentation has been updated if adding/changing functionality.

@elijahbenizzy elijahbenizzy changed the title Wires through application context to nodes that request it Forking/parallelism tracking (WIP) May 28, 2024
@elijahbenizzy
Copy link
Contributor Author

Also fixes #211

@elijahbenizzy elijahbenizzy force-pushed the spawn-applications branch 2 times, most recently from cacb9c3 to 467f484 Compare May 31, 2024 05:09
burr/core/application.py Show resolved Hide resolved
burr/core/application.py Show resolved Hide resolved
burr/core/application.py Outdated Show resolved Hide resolved
burr/lifecycle/base.py Outdated Show resolved Hide resolved
burr/tracking/client.py Outdated Show resolved Hide resolved
burr/tracking/common/models.py Outdated Show resolved Hide resolved
telemetry/ui/src/components/routes/AppList.tsx Outdated Show resolved Hide resolved
@elijahbenizzy elijahbenizzy force-pushed the spawn-applications branch 7 times, most recently from dadd91b to 3c619fe Compare May 31, 2024 21:49
@elijahbenizzy elijahbenizzy changed the title Forking/parallelism tracking (WIP) Forking/parallelism tracking May 31, 2024
@@ -42,6 +43,9 @@
from burr.lifecycle.base import LifecycleAdapter
from burr.lifecycle.internal import LifecycleAdapterSet

if TYPE_CHECKING:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO -- double-check that this is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove it later but this is OK for now. The other one we do an in-line import, but this is for type-checking

burr/core/application.py Outdated Show resolved Hide resolved
burr/core/application.py Show resolved Hide resolved
burr/tracking/client.py Show resolved Hide resolved
@@ -1640,7 +1640,7 @@
"name": "python",
"nbconvert_exporter": "python",
"pygments_lexer": "ipython3",
"version": "3.10.4"
"version": "3.11.6"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO -- change this back

notebook.ipynb Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove

@elijahbenizzy elijahbenizzy force-pushed the spawn-applications branch 2 times, most recently from e1bca8b to 1148692 Compare June 1, 2024 03:57
A node can now query for application context by setting an input
as __context -- this is of type ApplicationContext. This will allow
forking (+ any other information we may need from inside the node).
This is one of two __ variables (the other being tracer).

The dependency factory is a bit in flux but the way these are
constructed is not yet public-facing (we cannot arbitrarily add global
injectors), so we can change it.

This also fixes a bug in which dependency factories were not allowed to
be optional/have defaults. This enables the user to build out a
function that can be run outside the app (with a null context)..
This enables an application to specify the app that spawned it.
This enables us to track from child app the parents that spawned it
This does a little trickiness with the local tracker, but will allow the
UI to display bidirectional links for forks/spawns.
This allows us to expand/focus on children in the app list.
This also hides the partition key if it doesn't exist

See #211
This adds the proper UI component + fixes logging/server to log
correctly when spawning.
This is potentially backwards compatible, but people should really not
be doing this -- in standard python convention (and burr, specifically),
these are reserved for internal/special cases.
This way we can just run tests locally
@elijahbenizzy
Copy link
Contributor Author

Killing this on in favor of the mega-one (easier to iterate on): #225

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant