Skip to content

A user can create a bundle that spans multiple files - #252

Merged
ivan-khomyakov-sc merged 5 commits into
hmi-releasefrom
ivan_k/pickle_modules
Mar 12, 2022
Merged

A user can create a bundle that spans multiple files#252
ivan-khomyakov-sc merged 5 commits into
hmi-releasefrom
ivan_k/pickle_modules

Conversation

@ivan-khomyakov-sc

@ivan-khomyakov-sc ivan-khomyakov-sc commented Mar 9, 2022

Copy link
Copy Markdown

https://app.shortcut.com/scaleai/story/396138/a-user-can-create-a-bundle-that-spans-multiple-files

Users can pickle bundles that rely on external modules. It's done by passing globals() value

client.create_model_bundle(..., globals_copy=globals())

@ivan-khomyakov-sc ivan-khomyakov-sc changed the title update cloudpickle; register_pickle_by_value A user can create a bundle that spans multiple files Mar 9, 2022
Comment thread pyproject.toml
rich = "^10.15.2"
shellingham = "^1.4.0"
scikit-learn = ">=0.24.0"
cloudpickle = "^1.6.0"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this mean we'll have to pin the version of cloudpickle inside of the async/sync inference endpoints to 2.0.0? Not entirely sure actually

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Nope!

Technically to my understanding and exploring the cloudpickle source code, you can use even simple pickle to deserialize

Comment thread poetry.lock
@@ -253,6 +253,46 @@ packaging = "*"
six = ">=1.9.0"
webencodings = "*"

@seanshi-scale seanshi-scale Mar 10, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ty for checking in poetry.lock lol (diego told me about this, I'm guessing he asked you as well)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

yeah, there was a weird bug with other packages. I guess we just need to call poetry add <package> instead of doing it manually in pyproject.toml

Comment thread nucleus/deploy/client.py
model: Optional[DeployModel_T] = None,
load_model_fn: Optional[Callable[[], DeployModel_T]] = None,
bundle_url: Optional[str] = None,
globals_copy: Optional[Dict[str, Any]] = None,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a reason that the client has to pass in globals().copy(), or can we get the value of that from inside this function? I think if there isn't a reason for the client to pass this in then we shouldn't make them do that

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

to my understanding no... There are some problems with using iterating globals() i.e. for k, v in globals.items() because size of globals changes while iterating. If you called globals from outside of the function and pass it as globals_copy then it's fine.

To answer you question: no it is not needed to .copy() it (I think). But I used this name to keep the same naming convention as in https://github.com/scaleapi/nucleus-python-client/blob/hmi-release/nucleus/deploy/find_packages.py#L323

Comment thread nucleus/deploy/client.py
# Avoid recursion
# register_pickle_by_value does not work properly with itself
continue
cloudpickle.register_pickle_by_value(module)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@yixu34 did this cloudpickle.register_pickle_by_value thing end up working? Thought it didnt?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I checked it with an experiment: having some code in another file different from where I defined the bundle.

There are some limits to this approach:

  • if you do import inside the bundle function, it won't work
  • you cannot use non-module imports aka from foo import woo. This is a known issue, but I thought solving it is a little overkill at this point. We'd need to go to low level

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hmm so it works just that the only limitation is you have to import the full module and outside the function?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@syandroo given that they say on cloudpickle page: "Note that this feature is still experimental" I wouldn't be very hopeful :)

At least a simple use case of multiple .py files works fine.

I guess the next step is to try on one of the scale ml projects

@ivan-khomyakov-sc
ivan-khomyakov-sc merged commit eb7f015 into hmi-release Mar 12, 2022
@ivan-khomyakov-sc
ivan-khomyakov-sc deleted the ivan_k/pickle_modules branch March 12, 2022 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants