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

Export dag #909

Merged
merged 32 commits into from
Sep 16, 2024
Merged

Export dag #909

merged 32 commits into from
Sep 16, 2024

Conversation

Leahh02
Copy link
Collaborator

@Leahh02 Leahh02 commented Aug 28, 2024

This is my work from the visualize-day branch, but it's now coming off of develop.

The goal is to export a dag as a graphml so it can be used by visualization tool.

@Leahh02 Leahh02 added the WIP Work in progress label Aug 28, 2024
@Leahh02
Copy link
Collaborator Author

Leahh02 commented Aug 28, 2024

The changes in commit 2e284d1 is what made the "NOT_RESPONDING" state appear the other day. I'll test this again.

@Leahh02
Copy link
Collaborator Author

Leahh02 commented Aug 28, 2024

I tested it again and the "NOT_RESPONDING" state didn't appear.

@pagrubel
Copy link
Collaborator

Thank you this is much easier to review.

@Leahh02 Leahh02 requested a review from aquan9 August 29, 2024 16:43
@Leahh02 Leahh02 added the WIP (lint-only) Only run the linter in CI, nothing else label Aug 29, 2024
@Leahh02 Leahh02 removed the WIP (lint-only) Only run the linter in CI, nothing else label Aug 29, 2024
@aquan9 aquan9 removed the WIP Work in progress label Sep 9, 2024
@aquan9 aquan9 self-assigned this Sep 9, 2024
aquan9
aquan9 previously requested changes Sep 9, 2024
Copy link
Collaborator

@aquan9 aquan9 left a comment

Choose a reason for hiding this comment

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

This is great work, thanks Leah.

One last thing I'd like to see (besides the other changes I noted) before putting my final stamp of approval is an addition to our beeflow client documentation, since this change adds a new command to the client.


if __name__ == "__main__":
# Path to your GraphML file
graphml_file = '/vast/home/leahh/.beeflow/dags/bebd6f.graphml'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove this user specific path, and replace it with a generic path for this PR.

generate_graph.py Outdated Show resolved Hide resolved
@aquan9 aquan9 assigned Leahh02 and unassigned aquan9 Sep 9, 2024
@Leahh02 Leahh02 mentioned this pull request Sep 9, 2024
@pagrubel
Copy link
Collaborator

pagrubel commented Sep 9, 2024

@Leahh02 uses an augmented neo4j container for this capability. I'd like to see the Dockerfile for it in our repo to have a record of what to build. We will also need to fix the dependency build to use that, but that can be done in another PR.
I was able to verify that merging this would not affect running with the basic neo4j container. The dag functionality won't work but workflows will.

@Leahh02 Leahh02 added the WIP (lint-only) Only run the linter in CI, nothing else label Sep 10, 2024
@Leahh02 Leahh02 removed the WIP (lint-only) Only run the linter in CI, nothing else label Sep 11, 2024
@Leahh02
Copy link
Collaborator Author

Leahh02 commented Sep 11, 2024

The clamr graph doesn't work, I'm trying to figure out why. I forgot to test it.

@Leahh02
Copy link
Collaborator Author

Leahh02 commented Sep 11, 2024

The clamr graph doesn't work, I'm trying to figure out why. I forgot to test it.

I can create a dag when I have submitted the workflow with the no-start flag, but once I've started the workflow I can't make the graph anymore. The graphml file gets created but there's problems with networkx.

@Leahh02 Leahh02 added WIP (no-ci) Don't run any CI for this PR and removed WIP (no-ci) Don't run any CI for this PR labels Sep 12, 2024
Copy link
Collaborator

@pagrubel pagrubel left a comment

Choose a reason for hiding this comment

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

This looks great! I made sure Andres's change requests were in this PR. I tested this and will run our local overnight tests, upon those passing I'll approve and merge.

@pagrubel pagrubel dismissed aquan9’s stale review September 16, 2024 19:44

I checked his requests and they have been done.

Copy link
Collaborator

@pagrubel pagrubel left a comment

Choose a reason for hiding this comment

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

Great job, thank you @Leahh02

@pagrubel pagrubel merged commit 3890a44 into develop Sep 16, 2024
6 checks passed
@pagrubel pagrubel deleted the export-dag branch September 16, 2024 21:05
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.

3 participants