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

[cleanup] Clear and make consistent "pytorch" model nature #450

Open
vkuzmin-uber opened this issue Oct 12, 2020 · 1 comment
Open

[cleanup] Clear and make consistent "pytorch" model nature #450

vkuzmin-uber opened this issue Oct 12, 2020 · 1 comment

Comments

@vkuzmin-uber
Copy link
Contributor

vkuzmin-uber commented Oct 12, 2020

It seems that PyTorch models in Neuropod universe allows some "variant reading". This is a result of historical reasons when neuropod didn't have separate Backend projects and user could care less about model "platform" type.

I can show in by exmaple of our project MADL that uses Neuropod since first releases.

This is how It creates PyTorch model:

from neuropod.backends.pytorch.packager import *
neuropod_path = "example/torch_addition/neuropod"
create_pytorch_neuropod(
    neuropod_path=neuropod_path,
    model_name="torch_addition_model",
...

Neuropod code and comment with TODOs

  from neuropod.backends.python.packager import create_python_neuropod

  # Since we can package a pytorch model by just treating it as python code,
  # create an alias
  # TODO: do we want to create a separate function for pytorch that just wraps
  # `create_python_neuropod`?
  # TODO: do we want to write different docstrings for python and pytorch?
  create_pytorch_neuropod = create_python_neuropod

Function name specific for "pytorch" and "TODOs" demonstrate that Neuropod had no final decision on the "pytorch" model.
As I can see our project started to treat Neuropod "pytorch" and "python" as different models. This is reflected in user facing APIs, docs and eventually in teams discussion (Neuropod PyTorch and Python models are separate, at the same time Neuropod PyTorch and Torchscript models can be references as Torch).
   
Is it correct to say that TODOs are not actual anymore and "pytorch" is not considered to be "different" in any context?

If this is true, Neuropod should support it consistently:

  • Remove and don't allow "pytorch" specific functions
  • make it clear in doc that "pytorch" is a "python model with torch module"
  • probably stress on "torchscript" platform/model are not close to "pytorch" in Neuropod internals.

Please let me know if I am not right about this issue.

@VivekPanyam
Copy link
Collaborator

In general, I agree with your points above, but I want to provide some additional context.

For people who aren't familiar with the internals of Neuropod, we don't want to imply that we don't have first-class support for PyTorch. If someone's just skimming docs and they see TensorFlow, Keras, TorchScript, and Python, it might not be immediately clear to them that Python contains PyTorch. Even if that's clear, they may assume that PyTorch doesn't have first-class support within Neuropod.

That was the main reason for not exposing only a Python backend.

Another reason for exposing Python and PyTorch separately was that it would be reasonable to argue that "pytorch models execute by using the python backend" could be an implementation detail.

However, that isn't true in the current implementation. The only difference between PyTorch and Python models at the moment is the documentation and some aliases in the code.

I think we should make it clear that PyTorch isn't any less important or less well-supported than any other framework before removing the aliases. Maybe:

  • Explicitly mention that we test PyTorch models in CI
  • Mention PyTorch anywhere we mention all the other frameworks (and have PyTorch specific sections)
  • Provide Python Neuropod examples that use PyTorch
  • Explain that "A PyTorch Neuropod is just a Python Neuropod that happens to use PyTorch"

We already do a lot of the above, but we need to make it a lot more clear.

If anyone has any thoughts or suggestions on this, please comment on this issue!

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

No branches or pull requests

2 participants