Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added first iteration of easy_publish #153

Merged
merged 56 commits into from
Sep 29, 2022
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
56 commits
Select commit Hold shift + click to select a range
828b678
remove exception chaining when given unknown type
isaac-darling Jul 21, 2022
c2af2f3
add easy_publish method to dlhub.client
isaac-darling Jul 21, 2022
9c6041f
fix style in easy_publish
isaac-darling Jul 21, 2022
4e0a17f
add missing whitespace
isaac-darling Jul 21, 2022
e16cc99
fix error message in HelpMessage
isaac-darling Jul 21, 2022
4812942
merge the two static init options into one method
isaac-darling Jul 22, 2022
9f299b8
reorder static create_model params for portability
isaac-darling Jul 22, 2022
4e58649
return the old model constructor for compatibility
isaac-darling Jul 24, 2022
760b703
remove walrus operator, fix help message, revert return to task id
isaac-darling Jul 25, 2022
8e1d3c6
patch publishing test to so it runs without bloat
isaac-darling Jul 25, 2022
03e948f
patch publishing test so that it runs without bloat
isaac-darling Jul 25, 2022
dc9fb36
add functionality for extracting metadata from hints
isaac-darling Jul 27, 2022
0524c89
add auto-inspecting to easy_publish
isaac-darling Jul 27, 2022
1fb6615
switch easy_publish test to a function with hints
isaac-darling Jul 27, 2022
3e573d2
add clarifying comments
isaac-darling Jul 31, 2022
d4e6232
remove commented test skips
isaac-darling Aug 9, 2022
981efe7
add affiliations parameter to easy_publish
isaac-darling Aug 9, 2022
5ba2463
add paper_doi parameter to easy_publish
isaac-darling Aug 11, 2022
026ef66
simplify paper_doi parameter of easy_publish
isaac-darling Aug 11, 2022
3521090
add clarifying comments and extract repeated code
isaac-darling Aug 12, 2022
af04041
Merge branch 'dev' into friendly-publish
isaac-darling Aug 15, 2022
802cfc0
Merge branch 'dev' into auto-extract-metadata
isaac-darling Aug 15, 2022
ba9e7e7
fix code style
isaac-darling Aug 15, 2022
0ecdbbc
add comments to bypass flake8
isaac-darling Aug 16, 2022
bf55004
add pytest-mock to testing-requirements
isaac-darling Aug 16, 2022
c93efeb
add new parameters to easy_publish test
isaac-darling Aug 19, 2022
e35b097
Merge pull request #163 from DLHub-Argonne/dev
ascourtas Aug 26, 2022
fcbd173
update version to 1.0.0 for funcx compat
ascourtas Aug 26, 2022
624a59b
switch to backwards compatible type hints
isaac-darling Sep 13, 2022
83c126f
switch to backwards compatible type hints
isaac-darling Sep 13, 2022
ac7f1d7
switch to backwards compatible type checking
isaac-darling Sep 13, 2022
873f34c
update test model to noop_v11 since v10 has been corrupted (#164)
ascourtas Sep 15, 2022
5e8d8c6
release for test update patch
ascourtas Sep 15, 2022
6b1af9c
Updating RTD Python version to 3.9
blaiszik Sep 15, 2022
1a77bfd
RTD Python to 3.8
blaiszik Sep 15, 2022
3b08006
Add numpy latest requirement
blaiszik Sep 15, 2022
dbc7126
Merge branch 'master' into friendly-publish
isaac-darling Sep 16, 2022
71cb851
Merge branch 'master' into auto-extract-metadata
isaac-darling Sep 16, 2022
5e5d34c
move inspect code to util module
isaac-darling Sep 16, 2022
9310f71
add support for typing type names
isaac-darling Sep 16, 2022
ee975d7
address the behavior of the type checker
isaac-darling Sep 16, 2022
f4ec3c2
Merge branch 'auto-extract-metadata' into friendly-publish
isaac-darling Sep 16, 2022
b1eac69
remove unnecessary comments
isaac-darling Sep 16, 2022
e8576bd
make auto-inspection the default behavior
isaac-darling Sep 16, 2022
08d255a
revert last commit
isaac-darling Sep 16, 2022
f7eb374
adjust publish test to use my function
isaac-darling Sep 16, 2022
ba0a4fe
update condition to isolate subscripted hints
isaac-darling Sep 16, 2022
7cbfded
add tests to improve coverage of python model
isaac-darling Sep 16, 2022
7213660
add test for auto inspecting class methods
isaac-darling Sep 16, 2022
21a2ea0
fix path to pickle file
isaac-darling Sep 16, 2022
88dab48
fix test to cooperate with pickle
isaac-darling Sep 16, 2022
a22de1d
add file for tests on inspect functions
isaac-darling Sep 20, 2022
a9ef0c3
ndarray hints are not subscriptable
isaac-darling Sep 21, 2022
62b77be
add tests for inspect library
isaac-darling Sep 21, 2022
666f5c8
add handling for ndarray shape of Any
isaac-darling Sep 21, 2022
4828b88
add tests for ndarrays of Any shape
isaac-darling Sep 21, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 65 additions & 0 deletions dlhub_sdk/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,13 @@
from mdf_toolbox.globus_search.search_helper import SEARCH_LIMIT
from funcx.sdk.client import FuncXClient

from dlhub_sdk.models.servables.keras import KerasModel
from dlhub_sdk.models.servables.pytorch import TorchModel
from dlhub_sdk.models.servables.python import PythonClassMethodModel
from dlhub_sdk.models.servables.python import PythonStaticMethodModel
from dlhub_sdk.models.servables.tensorflow import TensorFlowModel
from dlhub_sdk.models.servables.sklearn import ScikitLearnModel

from dlhub_sdk.config import DLHUB_SERVICE_ADDRESS, CLIENT_ID
from dlhub_sdk.utils.futures import DLHubFuture
from dlhub_sdk.utils.schemas import validate_against_dlhub_schema
Expand All @@ -24,6 +31,10 @@
logger = logging.getLogger(__name__)


class HelpMessage(Exception):
"""Raised from another error to provide the user an additional message"""


class DLHubClient(BaseClient):
"""Main class for interacting with the DLHub service

Expand Down Expand Up @@ -336,6 +347,60 @@ 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]):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think adding an option for users to pass in a dict of datacite metadata if they'd like to (so have it be an optional dict parameter) would be ideal. How do you feel @WardLT ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

"""Simplified publishing method for servables

Args:
title (string): title for the servable
creators (string | list): either the creator's name (FamilyName, GivenName) or a list of the creators' names
short_name (string): shorthand name for the servable
servable_type (string): the type of the servable, must be a member of ("static_method",
"class_method",
"keras",
"pytorch",
"tensorflow",
"sklearn") more information on servable types can be found here:
https://dlhub-sdk.readthedocs.io/en/latest/servable-types.html
serv_options (dict): the servable_type specific arguments that are necessary for publishing
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Returns:
(string): task id of this submission, can be used to check for success
Raises:
ValueError: If the given servable_type is not in the list of acceptable types
Exception: If the serv_options are incomplete or the request to publish results in an error
"""
# conversion table for model string names to classes
models = {"static_method": PythonStaticMethodModel,
"class_method": PythonClassMethodModel,
"keras": KerasModel,
"pytorch": TorchModel,
"tensorflow": TensorFlowModel,
"sklearn": ScikitLearnModel}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

raise ValueError(f"dl.easy_publish given invalid servable type: {servable_type}, please refer to the docstring")

# attempt to construct the model and raise a helpful error if needed
try:
model_info = model.create_model(**serv_options)
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@isaac-darling isaac-darling Jul 29, 2022

Choose a reason for hiding this comment

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

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.


# set the required datacite fields
model_info.set_title(title)
creators = [creators] if isinstance(creators, str) else creators # handle the case where creators is a string
model_info.set_creators(creators, []) # we do not ask for affiliations
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

model_info.set_name(short_name)

# perform the publish
task_id = self.publish_servable(model_info)

# return the id of the publish task
Copy link
Contributor

Choose a reason for hiding this comment

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

great comments in this method Isaac, keep it up! 🎉

return task_id

def publish_servable(self, model):
"""Submit a servable to DLHub

Expand Down
19 changes: 14 additions & 5 deletions dlhub_sdk/models/servables/python.py
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,8 @@ class PythonStaticMethodModel(BasePythonServableModel):
"""

@classmethod
def create_model(cls, module, method, autobatch=False, function_kwargs=None):
"""Initialize the method
def create_model(cls, module=None, method=None, autobatch=False, function_kwargs=None, *, f=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

I had to google this use of * on its own in a function definition -- learn something new every day! So f here is a keyword-only argument, yes? Out of curiosity, what was the reasoning for not allowing it to be keyword or positional (vs exclusively keyword)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A little hard to explain in a comment here, but it's an issue of backwards compatibility for the create_model function. f is an entirely new parameter in this version of the function, so to avoid breaking old code the positions of the existing parameters need to remain the same. It would look weird then to use positionally, because you would have to specify None for the parameters you wanted to ignore. I hope this made sense.

"""Initialize the method based on the provided arguments
ascourtas marked this conversation as resolved.
Show resolved Hide resolved

Args:
module (string): Name of the module holding the function
Expand All @@ -163,7 +163,17 @@ def create_model(cls, module, method, autobatch=False, function_kwargs=None):
Calls :code:`map(f, list)`
function_kwargs (dict): Names and values of any other argument of the function to set
the values must be JSON serializable.
f (object): Function pointer to the Python function to be published
Raises:
TypeError: If there is no valid way to process the given arguments
"""
# determine which set of acceptable parameters was given, or err if invalid (valid would be f | module & method)
if f is not None:
ascourtas marked this conversation as resolved.
Show resolved Hide resolved
module, method = f.__module__, f.__name__ # if f is provided, assign its module and method names
elif module is None or method is None:
raise TypeError("PythonStaticMethodModel.create_model was not provided valid arguments. Please provide either a funtion pointer"
" or the module and name of the desired static function")

output = super(PythonStaticMethodModel, cls).create_model(method, function_kwargs)

output.servable.methods["run"].method_details.update({
Expand All @@ -175,13 +185,12 @@ def create_model(cls, module, method, autobatch=False, function_kwargs=None):
@classmethod
def from_function_pointer(cls, f, autobatch=False, function_kwargs=None):
"""Construct the module given a function pointer

Args:
f (object): A function pointer
f (object): Function pointer to the Python function to be published
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)
Copy link
Contributor

Choose a reason for hiding this comment

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

what to do about autobatch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure that I understand..


def _get_handler(self):
return 'python.PythonStaticMethodServable'
Expand Down
25 changes: 21 additions & 4 deletions dlhub_sdk/tests/test_dlhub_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import mdf_toolbox
from pytest import fixture, raises, mark
from pytest_mock import mocker # your editor may fail to detect its usage

from dlhub_sdk.models.servables.python import PythonStaticMethodModel
from dlhub_sdk.utils.futures import DLHubFuture
Expand Down Expand Up @@ -73,9 +74,9 @@ def test_run(dl):
assert res.result(timeout=60) == 'Hello world!'


@mark.skipif(not is_gha, reason='Avoid running this test except on larger-scale tests of the system')
@mark.skip
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):
Copy link
Contributor

Choose a reason for hiding this comment

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

nice job with mocking

Copy link
Contributor

Choose a reason for hiding this comment

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

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

# Make an example function
model = PythonStaticMethodModel.create_model('numpy.linalg', 'norm')
model.dlhub.test = True
Expand All @@ -84,8 +85,24 @@ def test_submit(dl):
model.set_inputs('ndarray', 'Array to be normed', shape=[None])
model.set_outputs('number', 'Norm of the array')

# make dummy reply for patch to return
class DummyReply:
def __init__(self) -> None:
self.status_code = 200

def json(self) -> dict[str, str]:
return {"task_id": "bf06d72e-0478-11ed-97f9-4b1381555b22"} # valid task id, status is known to be FAILED

# patch requests.post
mocker.patch("requests.post", return_value=DummyReply())

# Submit the model
dl.publish_servable(model)
task_id = dl.publish_servable(model)
assert task_id == "bf06d72e-0478-11ed-97f9-4b1381555b22"

# Submit the model using easy publish
task_id = dl.easy_publish("Norm of a 1D Array", "Developer, Some", "1d_norm", "static_method", {"module": "numpy.linalg", "method": "norm"})
assert task_id == "bf06d72e-0478-11ed-97f9-4b1381555b22"


def test_describe_model(dl):
Expand Down
2 changes: 1 addition & 1 deletion dlhub_sdk/utils/validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@isaac-darling isaac-darling Aug 9, 2022

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

Shows that I know. Let's keep yours



def _generate_err(err_type: Exception, path: List[Tuple[str, Hashable]], expected: type = None, given: type = None, *, msg: str = None) -> Exception:
Expand Down