Support keyword-only arguments #116

jl-wynen · 2024-02-02T12:40:25Z

Fixes #76

jl-wynen · 2024-02-02T12:42:53Z

src/sciline/pipeline.py

@@ -422,12 +406,12 @@ def set_param_table(self, params: ParamTable) -> None:
            for index, label in zip(params.index, values):
                self._set_provider(
                    Item((Label(tp=params.row_dim, index=index),), param_name),
-                    lambda label=label: label,
+                    _provider_for_constant(label),


Note the logic in ArgSpec.from_provider, implements a hard requirement for not allowing arguments without type hints as per #115. Thus, the old lambda does not work any more.

jl-wynen · 2024-02-02T12:44:57Z

tests/conftest.py

I think it makes sense to expand the use of these fixtures or control the default scheduler globally. As it stands, tests either use dask or the naive scheduler depending on whether dask is installed. This makes it hard to understand how the tests are executed.

SimonHeybrock · 2024-02-05T04:20:01Z

There are a bunch of places in visualize and iirc also in the html formatting that check for the internal lambda to detect certain things. That needs updating.

SimonHeybrock

Looks good! Minor questions (aside from the required fixes for visualize, etc.).

SimonHeybrock · 2024-02-05T05:31:35Z

src/sciline/scheduler.py

+                apply,
+                provider,
+                list(args.args),
+                (dict, [[key, val] for key, val in args.kwargs]),


A comment would be useful, it is not obvious for a reader what is going on here.

SimonHeybrock · 2024-02-05T05:34:09Z

tests/pipeline_test.py

@@ -1208,3 +1208,43 @@ def process(
 def test_html_repr() -> None:
    pipeline = sl.Pipeline([make_int], params={float: 5.0})
    assert isinstance(pipeline._repr_html_(), str)
+
+
+def test_pipeline_keyword_argument_and_param(scheduler: sl.scheduler.Scheduler) -> None:


Can you name this (and below) with something like keyword_only in the test name? I think keyword args have always been supported?

SimonHeybrock · 2024-02-05T05:39:09Z

src/sciline/_provider.py

+    def call(self, provider: Provider, values: dict[Key, Any]) -> Any:
+        """Call a compatible provider with arguments extracted from ``values``."""
+        return provider(
+            *(values[arg] for arg in self._args.values()),
+            **{key: values[arg] for key, arg in self._kwargs.items()},
+        )


Does this belong here? It seems it is used only by NaiveScheduler?

True, I don't like this one either

jl-wynen · 2024-02-05T08:38:54Z

There are a bunch of places in visualize and iirc also in the html formatting that check for the internal lambda to detect certain things. That needs updating.

I considered also introducing a dedicated Provider class. This would encapsulate these checks. E.g., when the class is instantiated, we could specify whether it is a regular provider, parameter, or series provider. Then we don't have to mess with detecting lambdas. Would this be a suitable change here?

SimonHeybrock · 2024-02-05T11:29:16Z

There are a bunch of places in visualize and iirc also in the html formatting that check for the internal lambda to detect certain things. That needs updating.

I considered also introducing a dedicated Provider class. This would encapsulate these checks. E.g., when the class is instantiated, we could specify whether it is a regular provider, parameter, or series provider. Then we don't have to mess with detecting lambdas. Would this be a suitable change here?

Sounds reasonable!

SimonHeybrock · 2024-02-06T04:34:16Z

src/sciline/task_graph.py

@@ -7,7 +7,8 @@

 from .scheduler import DaskScheduler, NaiveScheduler, Scheduler
 from .typing import Graph, Item
-from .utils import keyname, kind_of_provider
+
+# from .utils import keyname, kind_of_provider


SimonHeybrock · 2024-02-06T04:34:47Z

src/sciline/utils.py

-    if qualname(p) == f'{qualname(Pipeline.set_param_table)}.<locals>.<lambda>':
-        return 'table'
-    return 'function'
+# def kind_of_provider(p: Callable[..., Any]) -> ProviderKind:


SimonHeybrock · 2024-02-06T04:36:49Z

src/sciline/visualize.py

-            f'{_qualname(Pipeline.set_param_table)}.<locals>.<lambda>',
-            unsatisfied,
-        ):
+        if formatted_p.kind in ('parameter', 'table', 'sentinel'):


What is sentinel here? And why are we still using unsatisfied above but not here?

SimonHeybrock · 2024-02-06T04:40:14Z

tests/pipeline_with_param_table_test.py

@@ -350,6 +350,7 @@ def test_can_groupby_by_requesting_series_of_series() -> None:
        Param1,
        {1: sl.Series(Row, {0: 4, 1: 5}), 3: sl.Series(Row, {2: 6})},
    )
+    print(list(pl._providers.values()))


SimonHeybrock · 2024-02-06T04:50:36Z

src/sciline/_provider.py

+
+
+ToProvider = Callable[..., Any]
+ProviderKind = Literal['function', 'parameter', 'series', 'table', 'sentinel']


Any reason why we use literals instead of, e.g., an enum?

It was like this and I didn't want to change too much. I don't mind either way

SimonHeybrock · 2024-02-06T04:52:26Z

src/sciline/_provider.py

+        )
+
+    @classmethod
+    def table(cls, param: Any) -> Provider:


I guess it is more like a table-cell than a table?

SimonHeybrock · 2024-02-06T04:52:41Z

src/sciline/_provider.py

+            arg_spec=ArgSpec.null(),
+            kind='table',
+            location=ProviderLocation(
+                name=f'table({type(param).__name__})', module='sciline'


Same question about the module as above.

SimonHeybrock · 2024-02-06T04:52:58Z

src/sciline/_provider.py

+
+    @property
+    def qualname(self) -> str:
+        # TODO merge with location


SimonHeybrock · 2024-02-06T04:54:31Z

src/sciline/pipeline.py

+            provider = Provider.from_function(provider)
+        self._set_provider(provider.deduce_key(), provider)


Why allow creation of the provider if the key cannot be deduced? Should the check be moved into from_function?

from_function can, and probably should do this check. But we need to check again in deduce_key because we cannot deduce non-function provider keys.

SimonHeybrock · 2024-02-06T04:57:04Z

src/sciline/pipeline.py

@@ -406,12 +418,12 @@ def set_param_table(self, params: ParamTable) -> None:
            for index, label in zip(params.index, values):
                self._set_provider(
                    Item((Label(tp=params.row_dim, index=index),), param_name),
-                    _provider_for_constant(label),
+                    Provider.table(label),
                )
        for index, label in zip(params.index, params.index):
            self._set_provider(
                Item((Label(tp=params.row_dim, index=index),), params.row_dim),


Not necessary, but could this clumsy key setup be absorbed into the Provider class, similar to the function case? Provider.table would obviously need more arguments.

Would you store the key in the provider? This would duplicate the key as we still need it in the graph / pipeline.

jl-wynen · 2024-02-06T11:53:32Z

I think I addressed all you comments.

SimonHeybrock · 2024-02-06T14:04:26Z

src/sciline/_provider.py

+    def table_row(cls, param: Any) -> Provider:
+        """Construct a provider that returns the label for a table row."""


I think it is not a row that is being returned, just a single cell (i.e., specific row and column).

It is a single value. E.g. in https://scipp.github.io/sciline/user-guide/parameter-tables.html#Computing-results-for-series-of-parameters it is a single int, i.e., the RunID, but not the Filename.

Yes, i.e., rename?

SimonHeybrock · 2024-02-06T14:05:11Z

src/sciline/_provider.py

+"""Callable that can be converted to a provider."""
+
+ProviderKind = Literal[
+    'function', 'parameter', 'series', 'table', 'sentinel', 'unsatisfied'


Is 'sentinel' still in use?

Yes, in

sciline/src/sciline/pipeline.py

Line 587 in 011f9bf

graph[tp] = _ParamSentinel(self._param_name_to_table_key[tp])

and

sciline/src/sciline/pipeline.py

Line 592 in 011f9bf

graph[tp] = _ParamSentinel(tp)

Ok, but it will never make it to, e.g., visualize, right?

It shouldn't.

SimonHeybrock · 2024-02-06T14:40:57Z

src/sciline/_provider.py

@@ -27,7 +27,7 @@
 """Callable that can be converted to a provider."""

 ProviderKind = Literal[
-    'function', 'parameter', 'series', 'table', 'sentinel', 'unsatisfied'
+    'function', 'parameter', 'series', 'table_label', 'sentinel', 'unsatisfied'


Why label? I thought that would refer to columns, or it at least feels unclear?

Because this is what it's called in the code:

sciline/src/sciline/pipeline.py

Line 421 in f24a62f

Provider.table_label(label),

Yes, but shouldn't that be renamed?

Don't know. Pipeline seems to use 'label' and 'index' interchangeably for tables. So we could rename it to index?

No, this is two different things. If you rename it to index it would mean an entry of the index column:

A param table has an "index" column, and one or more data columns. Columns are labeled using a type.

The index is a bit like an index in a pandas.DataFrame (and it also has a column label, the type of the index).

A cell in the table is identified using an index value, i.e., row index (an index, in the alternate meaning of the word index, i.e., an entry of the index column) and a column label.

You renamed the method to table_cell, but here it is still table_label.

jl-wynen added 10 commits February 1, 2024 15:13

Basic support for kwarg with dask

24452df

Basic support for kwarg with dask

36b4695

Basic support for kwarg with naive scheduler

965d212

Dedicated ArgSpec class

254ad85

Store pos args as dict

fa9ce4b

Update some usage of Graph

1cf8184

Fix how series keys are resolved

686e27f

Fix uses of ArgSpec with Series

58b3b00

Test parameter series with kwargs

0311569

Fix type hints

ba11f3d

jl-wynen requested a review from SimonHeybrock February 2, 2024 12:40

jl-wynen commented Feb 2, 2024

View reviewed changes

SimonHeybrock reviewed Feb 5, 2024

View reviewed changes

SimonHeybrock changed the title ~~Support keyword arguments~~ Support keyword-only arguments Feb 5, 2024

SimonHeybrock mentioned this pull request Feb 5, 2024

Earlier detection of missing type hints #115

Closed

jl-wynen added 3 commits February 5, 2024 14:48

Rename to keyword_only

386e014

Explain dask graph syntax

357866a

Use dedicated Provider class

8919135

SimonHeybrock reviewed Feb 6, 2024

View reviewed changes

jl-wynen added 6 commits February 6, 2024 09:11

Fix getting function in dask scheduler

9a7b2b5

Fix TaskGraph HTML repr

8dcaa82

Fix detection of unsatisfied requirements

6f10f90

Remove debug print

1d16e5f

Use parameter module instead of sciline

5810cc4

Docstrings for Provider

1382afd

Fix type hints

011f9bf

SimonHeybrock reviewed Feb 6, 2024

View reviewed changes

jl-wynen mentioned this pull request Feb 6, 2024

Reimplement ORSO filewriter scipp/essreflectometry#27

Merged

Rename table -> table_label

f24a62f

SimonHeybrock reviewed Feb 6, 2024

View reviewed changes

Rename to table_cell

b7a160e

jl-wynen force-pushed the support-kwargs branch from 5941e05 to b7a160e Compare February 7, 2024 11:47

SimonHeybrock approved these changes Feb 7, 2024

View reviewed changes

jl-wynen merged commit c725afd into main Feb 7, 2024
5 checks passed

jl-wynen deleted the support-kwargs branch February 7, 2024 12:45

SimonHeybrock mentioned this pull request Feb 8, 2024

Provide access to code of providers via pipeline object #98

Open

jl-wynen mentioned this pull request Feb 15, 2024

Support decorated providers #127

Closed



		ToProvider = Callable[..., Any]
		ProviderKind = Literal['function', 'parameter', 'series', 'table', 'sentinel']

		provider = Provider.from_function(provider)
		self._set_provider(provider.deduce_key(), provider)

		def table_row(cls, param: Any) -> Provider:
		"""Construct a provider that returns the label for a table row."""

Support keyword-only arguments #116

Support keyword-only arguments #116

Conversation

jl-wynen commented Feb 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonHeybrock commented Feb 5, 2024

SimonHeybrock 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

jl-wynen commented Feb 5, 2024

SimonHeybrock commented Feb 5, 2024

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

jl-wynen commented Feb 6, 2024

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

jl-wynen Feb 6, 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

jl-wynen Feb 6, 2024 •

edited

Loading