feat: resolve relative datasample spec path #392

ThibaultFy · 2023-10-12T09:04:27Z

Related issue

See slack https://owkin.slack.com/archives/C01UWGW68N8/p1696943277910689 for context
closes FL-1243

Summary

Resolve path and paths on datasample spec.

jeandut · 2023-10-12T09:11:11Z

substra/sdk/schemas.py

+            path = pathlib.Path(values["path"])
+            if not path.is_absolute():
+                values["path"] = path.resolve()
+


else: values["path"]=path

linear · 2023-10-12T09:21:06Z

FL-1243 Resolve path in DatasampleSpec

Context

See slack with Jean for context: https://owkin.slack.com/archives/C01UWGW68N8/p1696943277910689

Specification

Relative path raise weird error if given to a datasample spec. We want to resolve the paths to allow relative path.

Acceptance criteria

thbcmlowk · 2023-10-12T10:31:09Z

substra/sdk/schemas.py

@@ -155,6 +155,23 @@ def exclusive_paths(cls, values):  # noqa: N805
            raise ValueError("'path' or 'paths' field must be set.")
        return values

+    @pydantic.model_validator(mode="before")
+    def resolve_paths(cls, values):  # noqa: N805


No typing on values?

I just copy paste it from abose. We might add it though, but it's values is provided by pydantic so it's not that bad no ? Wdyt ?

I would rather use values: Any): -> Any or create a custom type (e.g. ValueType) and use values: ValueType) -> ValueType:

guilhem-barthes

Thanks for your work!

guilhem-barthes · 2023-10-23T11:29:03Z

substra/sdk/schemas.py

+            paths = []
+            for path in values["paths"]:
+                path = pathlib.Path(path)
+                paths.append(path.resolve()) if not path.is_absolute() else paths.append(path)


Suggested change

paths.append(path.resolve()) if not path.is_absolute() else paths.append(path)

paths.append(path.resolve())

Do we need to check is_absolute() aspath.resolve() will return itself if already a absolute path?

Very good point ahah :D

guilhem-barthes · 2023-10-23T11:34:57Z

substra/sdk/schemas.py

@@ -155,6 +155,23 @@ def exclusive_paths(cls, values):  # noqa: N805
            raise ValueError("'path' or 'paths' field must be set.")
        return values

+    @pydantic.model_validator(mode="before")
+    def resolve_paths(cls, values):  # noqa: N805


I would rather use values: Any): -> Any or create a custom type (e.g. ValueType) and use values: ValueType) -> ValueType:

Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>

guilhem-barthes · 2023-10-26T07:44:30Z

substra/sdk/schemas.py

@@ -147,14 +147,31 @@ def is_many(self):
        return self.paths and len(self.paths) > 0

    @pydantic.model_validator(mode="before")
-    def exclusive_paths(cls, values):  # noqa: N805
+    def exclusive_paths(cls, values: typing.Any) -> typing.Any:  # noqa: N805


can you remove the# noqa: N805?

Nop, its because it does not like that the first arg is not "self" :/

If this method receive a class as first argument, pydantic recommends decorate with @classmethod for a proper type checking (model_validator)

nice thx :)

guilhem-barthes · 2023-10-26T07:45:00Z

substra/sdk/schemas.py

        """Check that one and only one path(s) field is defined."""
        if "paths" in values and "path" in values:
            raise ValueError("'path' and 'paths' fields are exclusive.")
        if "paths" not in values and "path" not in values:
            raise ValueError("'path' or 'paths' field must be set.")
        return values

+    @pydantic.model_validator(mode="before")
+    def resolve_paths(cls, values: typing.Any) -> typing.Any:  # noqa: N805


can you remove the # noqa: N805? It should be handled by the typing

guilhem-barthes · 2023-10-26T07:48:25Z

substra/sdk/schemas.py

        if "paths" in values and "path" in values:
            raise ValueError("'path' and 'paths' fields are exclusive.")
        if "paths" not in values and "path" not in values:
            raise ValueError("'path' or 'paths' field must be set.")


Could they define both keys but with value None?

I don't get what you're refering too. Could you provide an example plz ?

It looks like we allow None as value on these fields, therefore if a user explicitely define only one of this key to None (e.g. DataSampleSpec(path=None, data_manager_keys=[]), the check "paths" not in values and "path" not in values will return False (as the key is present, but the value is None).

Ok I see. I added some tests, and it appears that this case is dealts with in the resolve path, as None will raise a TypeError. This is not an explicit solution but I added the tests to ensure that this behavior is taken into account. What do you think ?

I don't know if TypeError is the clearest for the user, but maybe it is worth opening its own PR to review how we manage this edge case?

Yep True... I rm the tests, and +1 to deal with this in a dedicated PR

Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>

guilhem-barthes

Thanks for your work!

Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>

jeandut reviewed Oct 12, 2023

View reviewed changes

ThibaultFy force-pushed the feat/resolve-relative-datasample-path branch from 7bbb126 to 34ea495 Compare October 12, 2023 09:23

ThibaultFy marked this pull request as ready for review October 12, 2023 09:25

ThibaultFy requested a review from a team as a code owner October 12, 2023 09:25

thbcmlowk reviewed Oct 12, 2023

View reviewed changes

guilhem-barthes reviewed Oct 23, 2023

View reviewed changes

ThibaultFy added 4 commits October 24, 2023 09:46

feat: resolve datasample paths

6092a2a

Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>

feat: resolve paths in datasamples spec

9cadf7d

Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>

chore: reviews

753bee0

Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>

chore: changelog

dcdbb7a

Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>

ThibaultFy force-pushed the feat/resolve-relative-datasample-path branch from 34ea495 to dcdbb7a Compare October 24, 2023 07:47

guilhem-barthes reviewed Oct 26, 2023

View reviewed changes

ThibaultFy requested review from guilhem-barthes and thbcmlowk October 30, 2023 16:38

ThibaultFy added 3 commits November 2, 2023 11:12

chore: pydantic add @classmethod

07f012c

Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>

chore: add tests

9f0e61d

Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>

chore: rm tests

ca7c149

Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>

guilhem-barthes approved these changes Nov 2, 2023

View reviewed changes

ThibaultFy merged commit 9ba1cf9 into main Nov 2, 2023
6 checks passed

ThibaultFy deleted the feat/resolve-relative-datasample-path branch November 2, 2023 13:36

ThibaultFy added a commit that referenced this pull request Jan 11, 2024

feat: resolve relative datasample spec path (#392)

29a4b0f

Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>

ThibaultFy added a commit that referenced this pull request Jan 11, 2024

feat: resolve relative datasample spec path (#392)

78762a9

Signed-off-by: ThibaultFy <50656860+ThibaultFy@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: resolve relative datasample spec path #392

feat: resolve relative datasample spec path #392

ThibaultFy commented Oct 12, 2023 •

edited

Loading

jeandut Oct 12, 2023 •

edited

Loading

linear bot commented Oct 12, 2023

Context

Specification

Acceptance criteria

thbcmlowk Oct 12, 2023

ThibaultFy Oct 12, 2023

guilhem-barthes Oct 23, 2023 •

edited

Loading

guilhem-barthes left a comment

guilhem-barthes Oct 23, 2023

ThibaultFy Oct 23, 2023

guilhem-barthes Oct 23, 2023 •

edited

Loading

guilhem-barthes Oct 26, 2023

ThibaultFy Oct 30, 2023

guilhem-barthes Nov 2, 2023 •

edited

Loading

ThibaultFy Nov 2, 2023

guilhem-barthes Oct 26, 2023

guilhem-barthes Oct 26, 2023

ThibaultFy Oct 30, 2023

guilhem-barthes Nov 2, 2023

ThibaultFy Nov 2, 2023

guilhem-barthes Nov 2, 2023

ThibaultFy Nov 2, 2023

guilhem-barthes left a comment

	paths.append(path.resolve()) if not path.is_absolute() else paths.append(path)
	paths.append(path.resolve())

feat: resolve relative datasample spec path #392

feat: resolve relative datasample spec path #392

Conversation

ThibaultFy commented Oct 12, 2023 • edited Loading

Related issue

Summary

jeandut Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

linear bot commented Oct 12, 2023

Context

Specification

Acceptance criteria

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilhem-barthes Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

guilhem-barthes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilhem-barthes Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilhem-barthes Nov 2, 2023 • 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

guilhem-barthes left a comment

Choose a reason for hiding this comment

ThibaultFy commented Oct 12, 2023 •

edited

Loading

jeandut Oct 12, 2023 •

edited

Loading

guilhem-barthes Oct 23, 2023 •

edited

Loading

guilhem-barthes Oct 23, 2023 •

edited

Loading

guilhem-barthes Nov 2, 2023 •

edited

Loading