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.

Sign up for GitHub

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

Jump to bottom

Example of Jaiqu using burr #5

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Example of Jaiqu using burr #5

wants to merge 6 commits into from

Conversation

skrawcz
Copy link

@skrawcz skrawcz commented Apr 4, 2024 •
edited
Loading

Hi I saw your hn post and thought it would be good candidate for burr!

See https://news.ycombinator.com/item?id=39917364

Feedback / thoughts welcome! Basically the mermaid diagram enticed me to try burr since the mapping should be pretty close -- I think I got to a reasonable spot -- but it could be better.

For those coming to view -- compare this with what's in the jaiqu.py file.

Here's a run in the burr UI:

Screen Shot 2024-04-03 at 10 45 53 PM

The two versions I created:

  1. Straight port of existing code
    jaiqu_port

  2. More granular port trying to get close to the mermaid diagram
    jaiqu_granular

skrawcz added 2 commits April 1, 2024 23:56
@skrawcz
Translating to burr
27c99c0 
Step (1) big actions.
Step (2) write smaller actions that mirror the flow chart.
@skrawcz
Adds two implementations of Jaiqu using Burr
8876091 
jaiqu_burr_straight_port takes the code and
maps it directly to burr.

jaiqu_burr_granular takes the straight
port and breaks it up into more actions.

Note: I was hoping to use the mermaid
diagram and do something reflecting that
but the logic in the mermaid diagram
doesn't really map to the code and
thus isn't a good thing to model -- but
the more granualr version does kind of
look like it.
skrawcz
skrawcz commented Apr 4, 2024
View reviewed changes
jaiqu/__init__.py Outdated Show resolved Hide resolved
@areibman
Copy link
Contributor

areibman commented Apr 5, 2024 •
edited
Loading

@skrawcz seems you've made a lot of changes. Can you clarify/amend:

  1. Why are there so many .xml files? Are they necessary for this?
  2. Could you either add a lint rc into the repo or undo all the changes where whitespace is getting changed but not affecting the code?
  3. Update gitignore for files that don't need to be in there
  4. I'd prefer a direct but optional implementation on top of Jaiqu.py if you're going to add Burr. It doesn't seem like a good idea to copy + paste 1 file into 3 separate ones. That, or create an examples directory for each.
  5. Which implementation is supposed to be better? Granular or straight?
  6. Would prefer any .PNGs just be rendered in README rather than just left in repo

Looks like a great idea, and it'd do a lot to help allay the complexity of Jaiqu.

areibman
areibman requested changes Apr 5, 2024
View reviewed changes
.idea/.gitignore Outdated Show resolved Hide resolved
jaiqu/__init__.py Outdated Show resolved Hide resolved
jaiqu/helpers.py Outdated Show resolved Hide resolved
jaiqu/jaiqu Outdated Show resolved Hide resolved
jaiqu/jaiqu.png Outdated Show resolved Hide resolved
jaiqu/jaiqu.py Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
jaiqu/jaiqu_burr_straight_port.py Outdated Show resolved Hide resolved
jaiqu/jaiqu_burr_granular.py Outdated Show resolved Hide resolved
jaiqu/jaiqu_burr_granular.py Outdated Show resolved Hide resolved
@skrawcz
Copy link
Author

skrawcz commented Apr 5, 2024

@skrawcz seems you've made a lot of changes. Can you clarify/amend:

  1. Why are there so many .xml files? Are they necessary for this?
  2. Could you either add a lint rc into the repo or undo all the changes where whitespace is getting changed but not affecting the code?
  3. Update gitignore for files that don't need to be in there
  4. I'd prefer a direct but optional implementation on top of Jaiqu.py if you're going to add Burr. It doesn't seem like a good idea to copy + paste 1 file into 3 separate ones. That, or create an examples directory for each.
  5. Which implementation is supposed to be better? Granular or straight?
  6. Would prefer any .PNGs just be rendered in README rather than just left in repo

Looks like a great idea, and it'd do a lot to help allay the complexity of Jaiqu.

Thanks for reviewing! This was a very quick and dirty PR -- as judged by the extra cruft that made it in. 😅 .

Part of this PR was to get your thoughts/feedback on it. I left the original jaiqu implementation in there for context as I was developing -- an actual implementation would as you suggest not duplicate all that code over and over. In terms of implementation is there one you prefer more? I was more or less mindlessly porting rather than thinking about an ideal structure...

skrawcz added 3 commits April 14, 2024 21:15
@skrawcz
Makes Jaiqu use Burr
9ebd308 
[Burr](https://github.com/dagworks-inc/burr) will enable someone to more easily debug
the state of the jaiqu application if it's giving nonsense back
or it hits the maxium number of retries.

The design decision here is to make a close port of the original
code. It could be broken down further, but that would require
more work and I wasn't sure it TQDM was something that
had to stay or not.

This is a 100% backwards compatible change with the
current API.

tests have been moved to live under /tests/ and match
the README.

The mermaid image has been replaced with the
actual application structure that Burr generates.
@skrawcz
Merge remote-tracking branch 'upstream/main'
45b731e 
# Conflicts:
#	jaiqu/jaiqu.py
#	pyproject.toml
#	requirements.txt
@skrawcz
Adds save_trace option to CLI
67347a7 
So that the CLI has parity with the Burr changes.
@skrawcz
Copy link
Author

skrawcz commented Apr 15, 2024 •
edited
Loading

@skrawcz seems you've made a lot of changes. Can you clarify/amend:

  1. Why are there so many .xml files? Are they necessary for this?
  2. Could you either add a lint rc into the repo or undo all the changes where whitespace is getting changed but not affecting the code?
  3. Update gitignore for files that don't need to be in there
  4. I'd prefer a direct but optional implementation on top of Jaiqu.py if you're going to add Burr. It doesn't seem like a good idea to copy + paste 1 file into 3 separate ones. That, or create an examples directory for each.
  5. Which implementation is supposed to be better? Granular or straight?
  6. Would prefer any .PNGs just be rendered in README rather than just left in repo

Looks like a great idea, and it'd do a lot to help allay the complexity of Jaiqu.

@areibman I think I fixed things up. As far as I have tested things work equivalently!

I would squash merge this change and have a commit like:

[Burr](https://github.com/dagworks-inc/burr) will enable someone to more easily debug
the state of the jaiqu application if it's giving nonsense back
or it hits the maxium number of retries.

The design decision here is to make a close port of the original
code. It could be broken down further, but that would require
more work and I wasn't sure it TQDM was something that
had to stay or not.

This is a 100% backwards compatible change with the
current API.

tests have been moved to live under /tests/ and match
the README.

The mermaid image has been replaced with the
actual application structure that Burr generates.

@skrawcz skrawcz requested a review from areibman April 15, 2024 04:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@areibman areibman Awaiting requested review from areibman

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@skrawcz @areibman