34 feature pyelk reformating and integration by LudoBroche · Pull Request #39 · ewoks-kit/ewoksdraw

LudoBroche · 2026-09-18T15:37:15Z

PR summary

Import elk algorithms into ewoksdraw

AI Disclosure

Claude used to learn how Maturin works and how to set up a project.
Claude used to assess licensing needs

codecov · 2026-09-18T15:40:37Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

LudoBroche · 2026-09-18T16:19:48Z

WIP: This PR is intended to give an overview of what each solution would imply.

Solution 1: Python implementation

Rewrite the required ELK layout algorithm in Python:

Implications

Pure Python.
Easy installation.
We maintain the layout algorithm ourselves.
Only the features we need are implemented, not the complete ELK behavior.

Solution 2: Use `elk-rs`

Use elk-rs through a small Rust/Python wrapper:

Rust wrapper: rust/src/lib.rs
Rust dependencies: Cargo.toml and Cargo.lock
Python API typing: src/ewoksdraw/_elk_rs.pyi
Maturin build configuration: pyproject.toml

Implications

Reuses the ELK implementation instead of maintaining our own algorithm.
Adds Rust, PyO3, Maturin, and elk-rs as maintenance dependencies.
Requires CI builds and tests on every supported platform.
Requires platform-specific wheels.
Development requires Rust and Cargo to be installed.

LudoBroche · 2026-09-18T16:20:55Z

@loichuder Please have a look at these two solutions described here. I would love to have your opinion.

LudoBroche · 2026-09-18T16:21:24Z

The actual rust wrapper

LudoBroche · 2026-09-18T16:22:28Z

+#[pyfunction]
+#[pyo3(signature = (graph_json, options_json = "{}"))]
+fn layout_json(graph_json: &str, options_json: &str) -> PyResult<String> {
+    layout_api::layout_json(graph_json, options_json).map_err(PyRuntimeError::new_err)
+}


We create here our pyfunction

Note this wrapper holds the Python GIL lock.
If necessary, we can improve that by releasing the GIL since Rust doesn't need it.
In the near future I don't think multi-threading of ewoksdraw will be common,

LudoBroche · 2026-09-18T16:22:43Z

+#[pymodule]
+fn _elk_rs(module: &Bound<'_, PyModule>) -> PyResult<()> {
+    module.add_function(wrap_pyfunction!(layout_json, module)?)?;
+    Ok(())
+}


And the python module

LudoBroche · 2026-09-18T16:23:23Z

+[dependencies.org-eclipse-elk-graph-json]
+git = "https://github.com/openedges/elk-rs.git"
+rev = "2191680292b9565592223c22a28b5ef33c3acbad"
+package = "org-eclipse-elk-graph-json"


elk-rs doesn't have an official crates and only lives in github.
So we pin the github

They have tags at least. Could we use a tag instead of revision?

LudoBroche · 2026-09-18T16:26:57Z

We call here our rust/python module and Python function to define the desired elk API.

LudoBroche · 2026-09-18T16:27:37Z

And we can use it directly

loichuder · 2026-09-28T09:20:39Z

@loichuder Please have a look at these two solutions described here. I would love to have our opinion.

I'd have liked to have yours before 😄

Personally, when I see the Python code of the layout, that looks like several hundred lines of code that I don't wish to maintain. I'd prefer to use the Rust binding.

LudoBroche · 2026-10-02T11:35:41Z

Agree, I'll go with the rust binding then.
I ll keep this branch and PR.

LudoBroche · 2026-10-02T14:56:21Z

I tested the rust-elk wrapper to generate the graphs.
Rusk-Elk uses a fancier algorithm for the tasks' placement, trying to make links direct and as short as possible.

Here is a comparison:

By tuning the option, we can get back to the old system, but I think I like the new compact way better.

@loichuder What your take on it ?

LudoBroche · 2026-10-07T11:57:56Z

Since we bundle elk/elk-rs.
I'm adding the licenses for it

LudoBroche · 2026-10-07T11:58:48Z

Explain the licensing,
MIT Ewoksdraw
EPL2 for the elk bindings

This is AI Generated I have no knowledge of licensing convention

LudoBroche · 2026-10-07T12:29:54Z

    id: str
    startPoint: ElkPoint
-    bendPoints: list[ElkPoint]
+    bendPoints: NotRequired[list[ElkPoint]]


A straight edge has no bend point.
In that case elk-rs ignore the key

LudoBroche · 2026-10-07T12:40:29Z

testing the binding.
Fake graph and making sure the routing of elks-rs behave as expected:

LudoBroche · 2026-10-07T13:00:30Z

@loichuder It's a little big for my liking; don't be scared to bother and look at it together

loichuder · 2026-10-08T06:41:06Z

Just merged my stack of PRs. Can you rebase?

LudoBroche · 2026-10-08T09:22:56Z

Just merged my stack of PRs. Can you rebase?
@loichuder Good for review

loichuder

Could you update CONTRIBUTING with the additional requirements to setup the project in dev mode? I think at minimum, there should be a rust installation but perhaps something else?

loichuder · 2026-10-09T12:13:17Z

+        result = _elk_rs.layout_json(json.dumps(graph))
+    except RuntimeError as e:
+        raise ElkLayoutError(str(e)) from e
+    return cast(ElkGraph, json.loads(result))


Do you think we should use a pydantic model instead of a dict for ElkGraph ? This would allow runtime validation and static typing.

loichuder · 2026-10-09T12:16:06Z

@@ -0,0 +1 @@
+def layout_json(graph_json: str, options_json: str = "{}") -> str: ...


Is this for mypy?

loichuder · 2026-10-09T12:18:05Z

+are reproduced without modification and included in the wheel and source
+distributions. The upstream notice describes the wider ELK project.
+
+When updating the elk-rs revision in `rust/Cargo.toml`, update these source


Should be part of CONTRIBUTING IMO

loichuder · 2026-10-09T12:20:04Z

+@pytest.mark.parametrize(
+    "graph_json, message",
+    [
+        ("{not json", "Failed to parse graph JSON"),
+        ("[]", "must be a json object"),
+        ("{}", "Every element must have an id"),
+    ],
+)
+def test_invalid_graph_raises(graph_json: str, message: str) -> None:
+    with pytest.raises(RuntimeError, match=message):
+        _elk_rs.layout_json(graph_json)


parametrize seems often useful but I feel that it hampers readability.

I think it would read better if this was three separate tests.

loichuder · 2026-10-09T12:22:38Z

+          path: dist
+
+      # No checkout and no Rust toolchain: only the wheel is tested
+      - name: Install wheel


Are we not testing the test from the sources anymore?

LudoBroche linked an issue Sep 18, 2026 that may be closed by this pull request

[Feature]: pyElk reformating and integration #34

Open

LudoBroche commented Sep 18, 2026

View reviewed changes

Comment thread rust/src/lib.rs

LudoBroche Sep 18, 2026

Copy link
Copy Markdown

Member Author

Choose a reason for hiding this comment

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

The actual rust wrapper

LudoBroche commented Sep 18, 2026

View reviewed changes

Comment thread src/ewoksdraw/tests/test_elk_backend.py

LudoBroche Sep 18, 2026

Copy link
Copy Markdown

Member Author

Choose a reason for hiding this comment

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

And we can use it directly

LudoBroche commented Oct 7, 2026

View reviewed changes

LudoBroche requested a review from loichuder October 7, 2026 12:59

LudoBroche added 10 commits October 8, 2026 11:09

Issue #34 - WIP - Importing layout computation in project

a5639aa

Issue #34 - Test with rustelk wrapper

ebfe164

Issue #34 - Using rust-elk for rounting

b308f67

Issue #34 - remove pyelk and add pytest

3e5b613

Issue #34 - Testing github workflow to build various wheels

0b0b898

Issue #34 - Adding arch

44f5733

Issue #34 - Adding metadata to Cargo.toml

17e2db1

Issue #34 - New name of the python-rust and license

0dfd0a5

Issue #34 - Adding linting and type for rust binding

28ed109

Issue #34 - renamed port_id

0045765

Issue #34 - Handling routing option

4a34c58

LudoBroche force-pushed the 34-feature-pyelk-reformating-and-integration branch from 27dd3ea to 4a34c58 Compare October 8, 2026 09:11

Issue #34 - rebase

d8acc0b

loichuder reviewed Oct 9, 2026

View reviewed changes

		@@ -0,0 +1 @@
		def layout_json(graph_json: str, options_json: str = "{}") -> str: ...

Conversation

LudoBroche commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR summary

AI Disclosure

Uh oh!

codecov Bot commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

LudoBroche commented Sep 18, 2026

Solution 1: Python implementation

Implications

Solution 2: Use elk-rs

Implications

Uh oh!

LudoBroche commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LudoBroche Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LudoBroche Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LudoBroche Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

loichuder commented Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LudoBroche commented Oct 2, 2026

Uh oh!

LudoBroche commented Oct 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LudoBroche Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LudoBroche Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LudoBroche commented Oct 7, 2026

Uh oh!

loichuder commented Oct 8, 2026

Uh oh!

LudoBroche commented Oct 8, 2026

Uh oh!

loichuder left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

LudoBroche commented Sep 18, 2026 •

edited

Loading

codecov Bot commented Sep 18, 2026 •

edited

Loading

Solution 2: Use `elk-rs`

LudoBroche commented Sep 18, 2026 •

edited

Loading

LudoBroche Sep 18, 2026 •

edited

Loading

LudoBroche Sep 18, 2026 •

edited

Loading

LudoBroche Sep 18, 2026 •

edited

Loading

loichuder commented Sep 28, 2026 •

edited

Loading

LudoBroche Oct 7, 2026 •

edited

Loading

LudoBroche Oct 7, 2026 •

edited

Loading