added first iteration of easy_publish #153

isaac-darling · 2022-07-21T18:23:58Z

DO-NOT-MERGE is added while additional features are considered and testing is conducted

ascourtas

great job Isaac, I think the easy_publish() method will be a huge step in making model publishing even more accessible to the user! I have several questions about code logic, etc. Also, I think allowing the user to set datacite metadata is important.

ascourtas · 2022-07-29T14:47:40Z

dlhub_sdk/client.py

+                                                                                   "pytorch",
+                                                                                   "tensorflow",
+                                                                                   "sklearn")
+            serv_options (dict): the servable_type specific arguments that are necessary for publishing


please add more context for the user on what the servable options are -- ideally, link to documentation

ascourtas · 2022-07-29T14:49:04Z

dlhub_sdk/client.py

+        except Exception as e:
+            help_err = HelpMessage(f"Help can be found here:\nhttps://dlhub-sdk.readthedocs.io/en/latest/source/dlhub_sdk.models.servables.html#"
+                                   f"{model.__module__}.{model.__name__}.create_model")
+            raise help_err from e


just to clarify, this help_err won't eat the original exception message, correct?

No. specifying from e means the traceback will show the original error and say something like "the following error is a direct result of the previous error" and then show the help message. docs on from Exception here.

ascourtas · 2022-07-29T14:51:38Z

dlhub_sdk/client.py

+
+        # raise an error if the provided servable_type is invalid
+        model = models.get(servable_type)
+        if model is None:


if you wanted to shorten this a bit, you could do if servable_type not in models -- slightly more pythonic imo

I would, but I also need to store the model variable for later. I suppose lookups are quick, but it's a little more visual clutter. up to you.

🤷‍♀️ I like "not in models" but not enough to change this here.

ascourtas · 2022-07-29T14:53:25Z

dlhub_sdk/client.py

+
+        # set the required datacite fields
+        model_info.set_title(title)
+        creators = [creators] if isinstance(creators, str) else creators


I see, so this is handling the case where the user inputs creators as a string instead of a list? please add a small inline comment saying as much (something like # handle creator as string or list or something similar)

ascourtas · 2022-07-29T14:54:26Z

dlhub_sdk/client.py

+        # set the required datacite fields
+        model_info.set_title(title)
+        creators = [creators] if isinstance(creators, str) else creators
+        model_info.set_creators(creators, [])  # we do not ask for affiliations


we should allow the user to still set affiliations if they'd like to

I'd be happy to talk about what parameters we want to provide for this function. When Logan and I discussed it, we opted to go for the absolute minimum and leave it to the publisher to edit later. All depends on where we want to draw the complexity line.

Let's do at least affiliations as a breakout argument.

dlhub_sdk/models/servables/python.py

ascourtas · 2022-07-29T18:18:48Z

dlhub_sdk/models/servables/python.py

        Args:
            f (object): A function pointer
            autobatch (bool): Whether to run function on an iterable of entries
            function_kwargs (dict): Any default options for this function
        """
-        return cls.create_model(f.__module__, f.__name__, autobatch, function_kwargs)
+        return cls.create_model(f=f, autobatch=autobatch, function_kwargs=function_kwargs)


what to do about autobatch?

I'm not sure that I understand..

dlhub_sdk/models/servables/python.py

ascourtas · 2022-07-29T18:20:33Z

dlhub_sdk/tests/test_dlhub_client.py

-def test_submit(dl):
+# @mark.skipif(not is_gha, reason='Avoid running this test except on larger-scale tests of the system')
+# @mark.skip
+def test_submit(dl, mocker):


nice job with mocking

Shall we go-ahead and remove the commented-out lines?

ascourtas · 2022-07-29T18:21:51Z

dlhub_sdk/utils/validation.py

@@ -36,7 +36,7 @@ def _type_name_to_type(name: str) -> Union[type, Tuple[type]]:
    try:
        return type_table.get(name) or __builtins__[name]  # getattr(__builtins__, name) if __builtins__ is a module
    except KeyError:  # AttributeError if __builtins__ is a module
-        raise ValueError(f"found an unknown type name in servable metadata: {name}")
+        raise ValueError(f"found an unknown type name in servable metadata: {name}") from None


what happens if you raise from None? I am not familiar with this pattern

It will raise the ValueError without mentioning that it was the result of a KeyError (the user does not need to know that) docs for exception chaining are here if you're interested.

The same behavior will happen without the "from None," (i.e., what we had before), if I understand right

From the docs: "Exception chaining happens automatically when an exception is raised inside an except or finally section. This can be disabled by using from None idiom." Since the ValueError is raised in an except chaining will happen automatically unless it is disabled (if I understand correctly).

Shows that I know. Let's keep yours

WardLT

I think we're ready after the minor changes here

WardLT · 2022-08-09T16:30:50Z

dlhub_sdk/client.py

@@ -336,6 +347,59 @@ def get_result(self, task_id, verbose=False):
            result = result[0]
        return result

+    def easy_publish(self, title: str, creators: Union[str, list[str]], short_name: str, servable_type: str, serv_options: dict[str, Any]):


I agree! We could still keep the "title" and "creators" as other arguments (that take precedent) so that people don't have to know datacite exists.

WardLT · 2022-08-09T16:31:42Z

dlhub_sdk/client.py

+
+        # raise an error if the provided servable_type is invalid
+        model = models.get(servable_type)
+        if model is None:


🤷‍♀️ I like "not in models" but not enough to change this here.

WardLT · 2022-08-09T16:32:10Z

dlhub_sdk/client.py

+        # set the required datacite fields
+        model_info.set_title(title)
+        creators = [creators] if isinstance(creators, str) else creators
+        model_info.set_creators(creators, [])  # we do not ask for affiliations


Let's do at least affiliations as a breakout argument.

WardLT · 2022-08-09T16:33:46Z

dlhub_sdk/tests/test_dlhub_client.py

-def test_submit(dl):
+# @mark.skipif(not is_gha, reason='Avoid running this test except on larger-scale tests of the system')
+# @mark.skip
+def test_submit(dl, mocker):


Shall we go-ahead and remove the commented-out lines?

WardLT · 2022-08-09T16:34:31Z

dlhub_sdk/utils/validation.py

@@ -36,7 +36,7 @@ def _type_name_to_type(name: str) -> Union[type, Tuple[type]]:
    try:
        return type_table.get(name) or __builtins__[name]  # getattr(__builtins__, name) if __builtins__ is a module
    except KeyError:  # AttributeError if __builtins__ is a module
-        raise ValueError(f"found an unknown type name in servable metadata: {name}")
+        raise ValueError(f"found an unknown type name in servable metadata: {name}") from None


The same behavior will happen without the "from None," (i.e., what we had before), if I understand right

isaac-darling · 2022-08-09T22:27:27Z

The changes you mentioned should be added now (aside from the datacite parameter, which I think gets at the overall ease of use/usefulness issue and warrants further consideration).

WardLT · 2022-08-09T22:33:20Z

Thanks! I'm happy then. Leaving off datacite for now is a good idea. We can add other fields in smaller PRs

Maybe another for now though: paper_doi so we can add a paper as an associated identifier

isaac-darling · 2022-08-11T18:49:17Z

I gathered what I could from the docs, please let me know if this is not what you were looking for

WardLT

Let's simplify the "paper_doi" even more

dlhub_sdk/client.py

isaac-darling · 2022-08-11T20:59:30Z

The relation is now fixed!

coveralls · 2022-09-16T19:09:03Z

Pull Request Test Coverage Report for Build 3101287839

180 of 190 (94.74%) changed or added relevant lines in 9 files are covered.
32 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.8%) to 89.737%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
dlhub_sdk/client.py	24	28	85.71%
dlhub_sdk/utils/tests/test_inspect.py	44	50	88.0%

Files with Coverage Reduction	New Missed Lines	%
dlhub_sdk/models/servables/init.py	1	93.18%
dlhub_sdk/models/servables/tests/test_tensorflow.py	11	76.0%
dlhub_sdk/models/servables/tensorflow.py	20	65.15%

Totals
Change from base Build 2929398202:	0.8%
Covered Lines:	1810
Relevant Lines:	2017

💛 - Coveralls

ascourtas · 2022-09-29T18:23:28Z

Looks fantastic, excellent work Isaac! Super pumped for these changes for the user

isaac-darling added 2 commits July 21, 2022 12:51

remove exception chaining when given unknown type

828b678

add easy_publish method to dlhub.client

c2af2f3

isaac-darling added the DO NOT MERGE label Jul 21, 2022

isaac-darling self-assigned this Jul 21, 2022

isaac-darling added 6 commits July 21, 2022 15:17

fix style in easy_publish

9c6041f

add missing whitespace

4e0a17f

fix error message in HelpMessage

e16cc99

merge the two static init options into one method

4812942

reorder static create_model params for portability

9f299b8

return the old model constructor for compatibility

4e58649

isaac-darling linked an issue Jul 25, 2022 that may be closed by this pull request

add a more user-friendly option for publishing servables #154

Closed

isaac-darling added 6 commits July 25, 2022 12:27

remove walrus operator, fix help message, revert return to task id

760b703

patch publishing test to so it runs without bloat

8e1d3c6

patch publishing test so that it runs without bloat

03e948f

add functionality for extracting metadata from hints

dc9fb36

add auto-inspecting to easy_publish

0524c89

switch easy_publish test to a function with hints

1fb6615

ascourtas requested changes Jul 29, 2022

View reviewed changes

add clarifying comments

3e573d2

WardLT approved these changes Aug 9, 2022

View reviewed changes

isaac-darling added 2 commits August 9, 2022 17:24

remove commented test skips

d4e6232

add affiliations parameter to easy_publish

981efe7

add paper_doi parameter to easy_publish

5ba2463

WardLT requested changes Aug 11, 2022

View reviewed changes

dlhub_sdk/client.py Outdated Show resolved Hide resolved

simplify paper_doi parameter of easy_publish

026ef66

add clarifying comments and extract repeated code

3521090

ascourtas and others added 16 commits September 15, 2022 15:39

update test model to noop_v11 since v10 has been corrupted (#164)

873f34c

release for test update patch

5e8d8c6

Updating RTD Python version to 3.9

6b1af9c

RTD Python to 3.8

1a77bfd

Add numpy latest requirement

3b08006

Merge branch 'master' into friendly-publish

dbc7126

Merge branch 'master' into auto-extract-metadata

71cb851

move inspect code to util module

5e5d34c

add support for typing type names

9310f71

address the behavior of the type checker

ee975d7

Merge branch 'auto-extract-metadata' into friendly-publish

f4ec3c2

remove unnecessary comments

b1eac69

make auto-inspection the default behavior

e8576bd

revert last commit

08d255a

adjust publish test to use my function

f7eb374

update condition to isolate subscripted hints

ba0a4fe

isaac-darling added 9 commits September 16, 2022 14:40

add tests to improve coverage of python model

7cbfded

add test for auto inspecting class methods

7213660

fix path to pickle file

21a2ea0

fix test to cooperate with pickle

88dab48

add file for tests on inspect functions

a22de1d

ndarray hints are not subscriptable

a9ef0c3

add tests for inspect library

62b77be

add handling for ndarray shape of Any

666f5c8

add tests for ndarrays of Any shape

4828b88

ascourtas approved these changes Sep 29, 2022

View reviewed changes

ascourtas merged commit 1bd1ef6 into dev Sep 29, 2022

isaac-darling deleted the friendly-publish branch October 4, 2022 14:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added first iteration of easy_publish #153

added first iteration of easy_publish #153

isaac-darling commented Jul 21, 2022

ascourtas left a comment •

edited

ascourtas Jul 29, 2022

ascourtas Jul 29, 2022

isaac-darling Jul 29, 2022 •

edited

ascourtas Jul 29, 2022

isaac-darling Jul 29, 2022

WardLT Aug 9, 2022

ascourtas Jul 29, 2022

ascourtas Jul 29, 2022

isaac-darling Jul 29, 2022

WardLT Aug 9, 2022

ascourtas Jul 29, 2022

isaac-darling Jul 29, 2022

ascourtas Jul 29, 2022

WardLT Aug 9, 2022

ascourtas Jul 29, 2022

isaac-darling Jul 29, 2022

WardLT Aug 9, 2022

isaac-darling Aug 9, 2022 •

edited

WardLT Aug 9, 2022

WardLT left a comment

WardLT Aug 9, 2022

WardLT Aug 9, 2022

WardLT Aug 9, 2022

WardLT Aug 9, 2022

WardLT Aug 9, 2022

isaac-darling commented Aug 9, 2022

WardLT commented Aug 9, 2022

isaac-darling commented Aug 11, 2022

WardLT left a comment

isaac-darling commented Aug 11, 2022

coveralls commented Sep 16, 2022 •

edited

ascourtas commented Sep 29, 2022

added first iteration of easy_publish #153

added first iteration of easy_publish #153

Conversation

isaac-darling commented Jul 21, 2022

ascourtas left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isaac-darling Jul 29, 2022 • edited

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

isaac-darling Aug 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WardLT 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

isaac-darling commented Aug 9, 2022

WardLT commented Aug 9, 2022

isaac-darling commented Aug 11, 2022

WardLT left a comment

Choose a reason for hiding this comment

isaac-darling commented Aug 11, 2022

coveralls commented Sep 16, 2022 • edited

Pull Request Test Coverage Report for Build 3101287839

💛 - Coveralls

ascourtas commented Sep 29, 2022

ascourtas left a comment •

edited

isaac-darling Jul 29, 2022 •

edited

isaac-darling Aug 9, 2022 •

edited

coveralls commented Sep 16, 2022 •

edited