Rewrite of Sciline's Pipeline #165

SimonHeybrock · 2024-05-08T07:39:27Z

Overview

This is a rewrite of Pipeline, as discussed in many places. Please see the rewrite.ipynb notebook for some context.

List of changes

TypeVars need to be constrained. The new mechanism eagerly inserts duplicate nodes based on this.
Mechanism to favor most-specialized provider is removed. The last provider being inserted is the one that is used. This may be a pitfall, but otherwise it is never clear if an insert should replace existing providers or not, and may even make it impossible to replace specialized providers with a generic one. The new implementation is very simple, so that is what we do.
Param table support removed, in favor of map and reduce based on Cyclebane. Not that groupby is not supported yet, but we have never used this in practice.
Provider input type hints must be unique.
__setitem__ and __getitem__ can be used to compose more complex graphs, sidestepping the dependency-injection-driven assembly of the base pipeline.

jl-wynen

I haven't reviewed DataGraph yet. That will need more time.

jl-wynen · 2024-05-08T13:10:36Z

pyproject.toml

@@ -30,6 +30,7 @@ requires-python = ">=3.10"
 # Run 'tox -e deps' after making changes here. This will update requirement files.
 # Make sure to list one dependency per line.
 dependencies = [
+  "cyclebane @ git+https://github.com/scipp/cyclebane@fix-conceptual-naming-issues",


You should merge this and release cyclebane before merging the current PR. I don't want to risk depending on a branch in a released package.

It still requires review.

jl-wynen · 2024-05-08T13:13:53Z

src/sciline/_provider.py

        self._args = args
        self._kwargs = kwargs
        self._return = return_

+    def __len__(self) -> int:


I don't think __len__ is the best choice here. The user might wonder whether this includes the return annotation or not. (This is how len(__annotations__) works)

By the same argument, isn't what existing methods like keys() return also confusing?

True. It should be more like argument_keys.

Since this is just an internal implementation detail, I suggest to leave this for now.

jl-wynen · 2024-05-08T13:14:46Z

src/sciline/_provider.py

+        out = self.map_keys(lambda arg: _bind_free_typevars(arg, bound=bound))
+        if self._return is not None:
+            out._return = _bind_free_typevars(self._return, bound=bound)
+        return out


Should this be moved into map_keys?

Wouldn't that make the method name misleading (since it is not only mapping the keys any more)?

See above. In Sciline, 'key' refers to the things that connect providers, i.e., domain types. So it applies to the return type as well.

But my question was in particular about whether it always makes sense to map the return key.

src/sciline/pipeline.py

jl-wynen · 2024-05-08T13:22:17Z

tests/task_graph_test.py

@@ -31,7 +31,7 @@ def as_float(x: int) -> float:

 def make_task_graph() -> Graph:
    pl = sl.Pipeline([as_float], params={int: 1})
-    return pl.build(float, handler=sl.HandleAsBuildTimeException())
+    return pl.build((float,), handler=sl.HandleAsBuildTimeException())


Is the interface of build and get different now? Why? This seems confusing.

build is a lower-level method with a simpler interface. The complexity of distinguishing between single and multiple keys was moved in to get. build should (as before) not be used by users.

Then why is it public?

jl-wynen · 2024-05-16T12:50:47Z

src/sciline/data_graph.py

I'm curious about performance. Did you check if the new solution is any slower than the old?
I know that building graphs should be fast compared to the actual computation. But given that this may be done per update in Beamlime, I would like to make sure that it doesn't add too much latency.

Cyclebane currently has some sub-optimal handling of graph creation after map-reduce. But I have not tested/benchmarked so far.

But given that this may be done per update in Beamlime

I don't think we should or plan to do that?

jl-wynen · 2024-05-16T12:58:50Z

src/sciline/data_graph.py

+    """Returns the set of all TypeVars in a type expression."""
+    if isinstance(t, TypeVar):
+        return {t}
+    return set(itertools.chain(*map(_find_all_typevars, get_args(t))))


The chain of map looked a bit odd to me at first. Is it equivalent to this?

Suggested change

return set(itertools.chain(*map(_find_all_typevars, get_args(t))))

return set.union(*map(_find_all_typevars, get_args(t)))

Can't get that to work, can you fix the suggested syntax?

src/sciline/data_graph.py

jl-wynen · 2024-05-16T13:24:05Z

src/sciline/data_graph.py

+        if key is NoneType:
+            raise ValueError('Key must not be None')
+        if key in self._graph:
+            self._graph.remove_edges_from(list(self._graph.in_edges(key)))


Suggested change

self._graph.remove_edges_from(list(self._graph.in_edges(key)))

self._graph.remove_edges_from(self._graph.in_edges(key))

?

I think NetworkX' in_edges returns a view, so that would break? @MridulS can correct me?

Yes it's the view of the dictionary underneath, so need to create a new list object to remove the edges.

jl-wynen · 2024-05-16T13:27:13Z

src/sciline/data_graph.py

+        """
+        # This is a questionable approach: Using MyGeneric[T] as a key will actually
+        # not pass mypy [valid-type] checks. What we do on our side is ok, but the
+        # calling code is not.


Can you elaborate why this fails in mypy?

Does this mean that every insertion of a param needs to have a type: ignore[valid-type] comment?

Apparently MyGeneric[T] is not a valid type. MyGeneric[MyType] is ok. So no, not every insertion needs a type: ignore[valid-type], just the less common ones using a generic (not specialized).

Would it be better to annotate __setitem__ with something more lenient? Maybe even Any? I don't like having legitimate usages flagged as errors. That is, I would prefer false negatives over false positives.

We should chat about that, as I have struggled quite a bit coming up with something sensible that is not just Any everywhere.

jl-wynen · 2024-05-16T13:30:57Z

src/sciline/data_graph.py

+
+    def __setitem__(self, key: Key, value: DataGraph | Any) -> None:
+        """
+        Provide a concrete value for a type.


Or a data graph.

src/sciline/data_graph.py

docs/developer/architecture-and-design/rewrite.ipynb

jl-wynen · 2024-05-16T13:53:15Z

docs/user-guide/generic-providers.ipynb

Can you add a bit of text explaining that type vars must have constraints?

jl-wynen · 2024-05-16T13:54:25Z

docs/user-guide/parameter-tables.ipynb

-    "Then we can compute `Result` for each index in the parameter table:"
+    "Note how we used a node name of the pipeline as the column name in the parameter table.\n",
+    "For convenience we used a `pandas.DataFrame` to represent the table above, but the use of Pandas is entirely optional.\n",
+    "Equivalently the table could be represented as a `dict`, where each key corresponds to a column header and each value is a list of values for that column.\n",


Can you add an example of this?

Co-authored-by: Jan-Lukas Wynen <[email protected]>

SimonHeybrock · 2024-05-30T11:01:55Z

Are there more comments here that cannot be done in a follow-up?

jl-wynen

I am fine with merging. But please make sure to add docstrings where missing in follow up work.

SimonHeybrock added 30 commits April 17, 2024 06:06

Prototype: Rewrite from scratch

593cde5

Support other graph in __setitem__

ab1ec1e

Handle Optional

b95e72f

Remove implemented plan

0082025

Add to do notes

92b0bda

Rename to DataGraph

8ecb956

Support generic providers with constraints

b882385

Support multiple targets in build

ce90f72

Fix some tests

1406595

Fix more tests

de6086d

Cleanup

98e0163

Cleanup

64a7f1d

Minor

9c5af48

Try with Cyclebane

d90c93f

Add notebook with examples and thoughts about rewrite

4f80dbd

Minor

06d5cdf

Wrap cyclebane.Graph

c7a90ed

Cleanup and remove Optional/Union handling

15f3d36

Cleanup

5d632d4

Add functions from untracked file

3003b0c

Update open questions

d541e35

Cleanup handling of reduce

aaaff1b

Strip old pipeline

8730a23

Reintroduce handler handling

aeaaf6c

Update syntax

f08c668

Revert removal of visualize

697a62d

Fix some smaller issues

0dc447e

Simplify

0421e47

Remove Series and ParamTable

eda0181

Fix more tests

797a610

SimonHeybrock added 5 commits May 8, 2024 05:07

Fix visualize for mapped graphs

335b9f9

Update param table docs

dbf7464

Add cyclebane dep and pass docs

e5ebefc

Mypy

e745fa6

Update design doc

52c1646

SimonHeybrock changed the title ~~V2 prototype~~ Rewrite of Sciline's Pipeline May 8, 2024

Merge remote-tracking branch 'origin/main' into v2-prototype

bcee03a

SimonHeybrock marked this pull request as ready for review May 8, 2024 08:27

SimonHeybrock requested a review from jl-wynen May 8, 2024 08:27

jl-wynen reviewed May 8, 2024

View reviewed changes

SimonHeybrock added 7 commits May 16, 2024 11:43

Map also return type

364a39a

Docstring

2faa71b

Mark method private

70fe2f2

Move __copy__

c91dab8

Remove build

cf3f1ca

Update cyclebane branch

ee9c3f2

Fix mypy

90ccffd

SimonHeybrock requested a review from jl-wynen May 16, 2024 12:04

jl-wynen reviewed May 16, 2024

View reviewed changes

SimonHeybrock mentioned this pull request May 21, 2024

Sciline rewrite: Overview of remaining tasks #166

Closed

2 tasks

SimonHeybrock and others added 4 commits May 21, 2024 06:30

Update src/sciline/data_graph.py

ec0d12b

Co-authored-by: Jan-Lukas Wynen <[email protected]>

Update docs/developer/architecture-and-design/rewrite.ipynb

bcca62a

Co-authored-by: Jan-Lukas Wynen <[email protected]>

Add note on typevars needing constraints

afc5c75

Give example using dict

c412cd1

SimonHeybrock mentioned this pull request May 21, 2024

Refactor to a cleaner implementation scipp/cyclebane#3

Merged

Use released cyclebane

a6872d8

jl-wynen approved these changes May 30, 2024

View reviewed changes

SimonHeybrock merged commit 4a7b218 into main Jun 3, 2024
5 checks passed

SimonHeybrock deleted the v2-prototype branch June 3, 2024 03:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite of Sciline's Pipeline #165

Rewrite of Sciline's Pipeline #165

SimonHeybrock commented May 8, 2024 •

edited

Loading

jl-wynen left a comment

jl-wynen May 8, 2024

SimonHeybrock May 13, 2024

jl-wynen May 8, 2024

SimonHeybrock May 13, 2024

jl-wynen May 16, 2024

SimonHeybrock May 16, 2024

jl-wynen May 8, 2024

SimonHeybrock May 13, 2024

jl-wynen May 16, 2024

jl-wynen May 8, 2024

SimonHeybrock May 13, 2024

jl-wynen May 16, 2024

jl-wynen May 16, 2024

SimonHeybrock May 21, 2024

jl-wynen May 16, 2024

SimonHeybrock May 21, 2024

jl-wynen May 16, 2024

SimonHeybrock May 21, 2024

MridulS May 23, 2024 •

edited

Loading

jl-wynen May 16, 2024

SimonHeybrock May 21, 2024

jl-wynen May 21, 2024

SimonHeybrock May 21, 2024

jl-wynen May 16, 2024

SimonHeybrock May 21, 2024

jl-wynen May 16, 2024

jl-wynen May 16, 2024

SimonHeybrock commented May 30, 2024

jl-wynen left a comment

	return set(itertools.chain(*map(_find_all_typevars, get_args(t))))
	return set.union(*map(_find_all_typevars, get_args(t)))

	self._graph.remove_edges_from(list(self._graph.in_edges(key)))
	self._graph.remove_edges_from(self._graph.in_edges(key))

Rewrite of Sciline's Pipeline #165

Rewrite of Sciline's Pipeline #165

Conversation

SimonHeybrock commented May 8, 2024 • edited Loading

Overview

List of changes

jl-wynen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MridulS May 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonHeybrock commented May 30, 2024

jl-wynen left a comment

Choose a reason for hiding this comment

SimonHeybrock commented May 8, 2024 •

edited

Loading

MridulS May 23, 2024 •

edited

Loading