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

Adds parent pointer storage + fork display in the UI #189

Merged
merged 4 commits into from
May 21, 2024

Conversation

elijahbenizzy
Copy link
Contributor

@elijahbenizzy elijahbenizzy commented May 17, 2024

This way we can know that an application was forked + link to it.

See #185.

Changes

Fork information is stored so we know parent pointers + can make debugging easier.

How I tested this

Manually, will add testing

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
Copy link
Contributor Author

elijahbenizzy commented May 17, 2024

TODO:

  • Add testing for this + maybe more E2E for forking
  • Move more datatypes to common (this will solve some other problems too

This way we can know that an application was forked + link to it.
This tests that we store + serialize parent pointers. There are a few
edge-cases we should probably handle (nulling out/tracking the sequence
ID, for example), but this is the 80/20 for now.
This is a bit ugly, but its a placeholder for a much cleaner future
refactor. I stand by haviung types.py here so we can centralize future
ones, even though only ParentPointer is there now.
@elijahbenizzy elijahbenizzy marked this pull request as ready for review May 18, 2024 20:27
@elijahbenizzy elijahbenizzy changed the title Adds parent pointer storage + fork display in the UI (WIP) Adds parent pointer storage + fork display in the UI May 19, 2024
Comment on lines +1578 to +1584
parent_pointer=burr_types.ParentPointer(
app_id=self.fork_from_app_id,
partition_key=self.fork_from_partition_key,
sequence_id=self.fork_from_sequence_id,
)
if self.loaded_from_fork
else None,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is okay here. It could be all in that one method though.

Copy link
Contributor

Choose a reason for hiding this comment

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

what's the diff here? did you just run the notebook? or?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, will remove

@@ -81,6 +82,21 @@ def from_transition(transition: Transition) -> "TransitionModel":
)


class PointerModel(IdentifyingModel):
Copy link
Contributor

@skrawcz skrawcz May 19, 2024

Choose a reason for hiding this comment

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

a short comment would help

Copy link
Contributor

@skrawcz skrawcz left a comment

Choose a reason for hiding this comment

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

haven't tested, but looks fine on first glance.

@elijahbenizzy elijahbenizzy merged commit b4158b3 into main May 21, 2024
8 checks passed
@elijahbenizzy elijahbenizzy deleted the add-fork-pointers-to-ui branch May 21, 2024 04:09
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

2 participants