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

Add DodoSource #68

Merged
merged 75 commits into from
May 9, 2023
Merged

Add DodoSource #68

merged 75 commits into from
May 9, 2023

Conversation

nrbgt
Copy link
Contributor

@nrbgt nrbgt commented Mar 20, 2023

References

Code changes

  • add some more demo deps
    • ipydatagrid
    • ipytree
      • already quite busy, wouldn't add much
    • ipylab
    • updated ipywidgets and jupyterlab_widgets
      • update serialization for strict JSON
  • add DodoSource
    • find nodes and lnks
      • tasks
      • files
      • subtasks
      • task dep
      • folders-of-files
  • example notebook
    • make panel
    • load source
    • make grids
    • link grids and selection
    • run tasks
      • logs without killing UI
      • without blocking kernel
    • highlight
      • dependency paths following file_dep and task_dep links
      • upstreams
      • downstreams
    • collapse nodes and edges
  • issues noticed while working on app
    • handle in-place updates (for certain cases)
    • handle linkHandlers and nodeHandlers better for facets
    • fix multiple shapes with "canary" attributes (e.g. Rectangle.width and Text.text)
    • add line_dash to more shapes (via HasStrokeAndFill)
    • fix center point of custom Rectangle and Ellipse shapes
  • docs
    • changelog
    • clean up some hard-coded positions for facets in docs
    • update jupyterlite-core and pyodide-kernel

User-facing changes

  • users of the binder demo will be able to try out the grid against an evolving system

  • users of the lit demo will see:

    note screen
    custom shapes image
    running tasks based on task selection image
    ipylab icons image
    shortest path with particles image
    upstream/downstream highlight image

Backwards-incompatible changes

  • n/a

@github-actions
Copy link

Try it on ⬅️ ReadTheDocs or Binder ➡️

Copy link
Contributor

@sanbales sanbales left a comment

Choose a reason for hiding this comment

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

Looking good, just some small things I ran into

src/ipyforcegraph/sources/dodo.py Outdated Show resolved Hide resolved
pyproject.toml Show resolved Hide resolved
examples/DodoSource.ipynb Outdated Show resolved Hide resolved
examples/DodoSource.ipynb Outdated Show resolved Hide resolved
examples/DodoSource.ipynb Outdated Show resolved Hide resolved
examples/DodoSource.ipynb Outdated Show resolved Hide resolved
nrbgt added a commit to nrbgt/ipyforcegraph that referenced this pull request Mar 23, 2023
@nrbgt nrbgt mentioned this pull request Apr 10, 2023
46 tasks
Copy link
Contributor

@sanbales sanbales left a comment

Choose a reason for hiding this comment

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

Looks good to me, just a couple of small comments, but happy to merge if you are cool with it.

- removes the significance of order in `ForceGraph.behaviors`
- all node, link, and graph behaviors now have a (sensible default) `.rank` trait
which determines the order in which they are applied.
- lower `rank` are applied before higher `rank`
- adds `DodoSource` for interpreting `doit` tasks graphs
- adds `node_preserve_columns`, `link_id_column`, and `link_preserve_columns`
- these allow for keeping values when updating data, such as those created by the
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wanted to double check this was still going in this PR...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep, it's in here, and the demo shows some improvement in the stability of the render, though the vanilla forces still encourage stuff to move

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't tell the difference between these two. Only bringing it up in case it was an unintentional commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it has just been compressed.

Copy link
Contributor

@sanbales sanbales left a comment

Choose a reason for hiding this comment

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

Ran a doit lab locally and played with the DodoApp.ipynb for a while.

Seems to be working well. Only things I've noticed (and these may be PEBCAK or not intended):

  1. selection from datagrid to graph does not seem to work (i.e., selecting a row on the grids, does not highlight nodes or links on the graph)
  2. sometimes the between particles do not seem to appear for things someone might expect (e.g., from env to setup)

Tried it on lite as well but didn't see the Dodo notebooks in there.

"""Draw a text shape, with an optional background."""
"""Draw a text shape, with an optional background.

If the ``text`` trait is (or evaluates to) ``0`` or ``None``, no shape will be drawn.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would the same happen with height?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it would visually hide it, but you'd still pay the canvas price

@nrbgt
Copy link
Contributor Author

nrbgt commented May 8, 2023

thanks for the review! 😍

didn't see the Dodo notebooks in there.

Yep, they don't work in lite because subprocess.Popen. Also doit doesn't import, due to some unvendored stdlib packages... to that end, I started some playground stuff on doitoml, but if course doit relies on subprocess.Popen as well, so only pure-python actions work.

the between particles

will look into it. Trying to get fancy with non-blocking stuff. Got some ideas on how that could be improved.

selection from datagrid

will look into it

@nrbgt
Copy link
Contributor Author

nrbgt commented May 8, 2023

selection from datagrid

will look into it

oof, this is general failure, even setting it programmatically doesn't work.

node_selection.selected = [0]
# doesn't highlight `all`

the issue is that we don't update the graphData.nodes._selected when a selection update comes in, and then node-selection thing is getting clobbered by the fancy text junk.

@nrbgt
Copy link
Contributor Author

nrbgt commented May 8, 2023

fixed selection on 43202f1

@nrbgt
Copy link
Contributor Author

nrbgt commented May 8, 2023

the between particles
will look into it.

Ah, this might due to it actually being a bit more accurate now, as it will only show particles if there is a path of the form:

[file] --> file_dep --> task --> targets --> [other file]

(Or the reverse)

Initially, it kinda lied, as it would follow sibling paths like:

[file] --> file_dep --> task --> file_dep --> [other file]

So basically: a lot of the nodes aren't particularly sensitive to other nodes. If there's a concrete example, happy to look into it!

@nrbgt
Copy link
Contributor Author

nrbgt commented May 9, 2023

I've updated the label to show the path count, a la image

@sanbales
Copy link
Contributor

sanbales commented May 9, 2023

the between particles
will look into it.

Ah, this might due to it actually being a bit more accurate now, as it will only show particles if there is a path of the form:

[file] --> file_dep --> task --> targets --> [other file]

(Or the reverse)

Initially, it kinda lied, as it would follow sibling paths like:

[file] --> file_dep --> task --> file_dep --> [other file]

So basically: a lot of the nodes aren't particularly sensitive to other nodes. If there's a concrete example, happy to look into it!

Yeah, that makes sense. No, I don't think it's worth it to go beyond what you've done. It's working nicely!

@sanbales
Copy link
Contributor

sanbales commented May 9, 2023

I vote we pull the trigger. The only thing I could find was that when you try to open some files, it says it can't find them. Think it is for files that start with .? Obviously, testing this on 🪟ers, so 🐧 may be fine...

image

@nrbgt nrbgt merged commit 08faf78 into jupyrdf:dev May 9, 2023
8 checks passed
@nrbgt nrbgt deleted the gh-44-dodo-source branch May 9, 2023 18:18
nrbgt added a commit to nrbgt/ipyforcegraph that referenced this pull request May 11, 2023
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