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

Basic PyTorch Integration #3069

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@jackm321
Copy link
Contributor

commented Jun 10, 2019

Summary:
Setup basics of model loader and import mul and div JIT IR nodes.
Still need to clean things up and figure out some things about the setup.py script.

Documentation:

Test Plan:
Python source:
Screen Shot 2019-06-10 at 8 26 09 AM

JIT IR:
Screen Shot 2019-06-10 at 8 25 14 AM

Glow graph:
Screen Shot 2019-06-10 at 8 24 46 AM

Results of running on JIT vs Glow match
Screen Shot 2019-06-10 at 8 29 28 AM

added a pytest test:
cd torch_glow && python setup.py test
Screen Shot 2019-06-21 at 11 55 00 AM

@yinghai

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

Cool! I think figuring our the testing CI is probably a major part of this. :)

@jackm321

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

@yinghai agreed, good CI testing will be critical here. Also CI is one of the motivations for locating the integration point in within the Glow repo because, with good testing in CI, we can more easily keep this interface from falling out of sync with Glow and becoming broken.

@zrphercule2

This comment has been minimized.

Copy link

commented Jun 11, 2019

Cool thing! Are we going to use circle ci as well?

(btw I am zrphercule and I will change back to my main account after I come back...)

@jackm321

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

@zrphercule2, yeah circleci should work for this

loadRes_->inputPlaceholders.push_back(loadValue(inputValue));
}

// Nodes are topologically sorted.

This comment has been minimized.

Copy link
@zrphercule2

zrphercule2 Jun 14, 2019

Is this always guaranteed by pytorch?

This comment has been minimized.

Copy link
@jackm321

jackm321 Jun 18, 2019

Author Contributor

As far as I know yes, but only know this from reading other code that depends on this property

@zrphercule2

This comment has been minimized.

Copy link

commented Jun 14, 2019

btw wondering what is the status of this pr? Went through it carefully, and looks great as a start.

@zrphercule2

This comment has been minimized.

Copy link

commented Jun 14, 2019

also, do we need another repo for this project?

@jackm321

This comment has been minimized.

Copy link
Contributor Author

commented Jun 14, 2019

@zrphercule2
status: I need to fix the build setup a bit, I should be able to get to it in the next working day or so
another repo: no I think we should keep it in Glow repo for easier development and keep it in sync with Glow repo

@jackm321 jackm321 force-pushed the jackm321:pytorch branch 4 times, most recently from 99f9eaf to 61615c4 Jun 18, 2019

@facebook-github-bot
Copy link

left a comment

@jackm321 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

for (size_t i = 0; i < inputs.size(); ++i) {
glow::Placeholder *ph = loaderResult->inputPlaceholders[i];
glow::TypeRef ty = ph->getType();
glow::Tensor t(inputs[i].toTensor().data_ptr(), ty);

This comment has been minimized.

Copy link
@zrphercule

zrphercule Jun 20, 2019

Member

where did we get the shape inference?

This comment has been minimized.

Copy link
@jackm321

jackm321 Jun 21, 2019

Author Contributor

It compiles on first run

torch::jit::Graph *subgraph)
: mod_(mod), subgraph_(subgraph) {}

// static

This comment has been minimized.

Copy link
@zrphercule

zrphercule Jun 20, 2019

Member

shall we have a static list for this?

This comment has been minimized.

Copy link
@jackm321

jackm321 Jun 21, 2019

Author Contributor

I don't think so, eventually this should probably look more like onnxifi's checkGraphCompatibility

This comment has been minimized.

Copy link
@zrphercule

zrphercule Jun 21, 2019

Member

I don't think so, eventually this should probably look more like onnxifi's checkGraphCompatibility

Check my comments lol

@zrphercule

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Overall LGTM, didnt reviewed the setup.py yet since I think you might want to change it. Feel free to ping me once you finished, and I will review all and give a stamp lol

@jackm321 jackm321 force-pushed the jackm321:pytorch branch 3 times, most recently from 24d02b4 to 4645091 Jun 21, 2019

Show resolved Hide resolved torch_glow/setup.py Outdated
@zrphercule
Copy link
Member

left a comment

LGTM! one more comments leave, and what is going on with the test, is it related?

@zrphercule

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

After this I think we can start to:

  1. build a working CI with standard tests for this.
    We would expect an onnx-test like test system, for each operator we support, we need to create unit test; also we need some e2e tests similar to test_basic.py. These tests should be run in a special "glow_pytorch" CI.
  2. start to add more ops includes Conv.
    Although using switch-case is acceptable for now since we only have very few ops supported/to-be-support, I think it will finally make our codebase dirty, since all op related works are in a few functions, and make other people confusing when they what to add new ops.
    I think we can instead make a global list/registry like what we have done in pytorch, caffe2 and onnx. for each op we support, we have a unique file to record all its related functionalities.

@jackm321 jackm321 force-pushed the jackm321:pytorch branch from 4645091 to ee02eb2 Jun 21, 2019

@facebook-github-bot
Copy link

left a comment

@jackm321 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@jackm321

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

build a working CI with standard tests for this.
We would expect an onnx-test like test system, for each operator we support, we need to create unit test; also we need some e2e tests similar to test_basic.py. These tests should be run in a special "glow_pytorch" CI.

100% agree, we should basically make an analog of the Caffe2ImporterTest in glow (but maybe have different tests in different files since each one will require a python program) and I agree we should also have some bigger end-end tests like model zoo models.

I think we can instead make a global list/registry like what we have done in pytorch, caffe2 and onnx. for each op we support, we have a unique file to record all its related functionalities.

This sounds different than how we import for example a c2 node as a subgraph in glow then try to lower it and run isOpSupported, is that the process you're talking about or something else. The tricky thing with PyTorch right now is that, unlike c2, we don't have shape information yet during this compatibility checking phase.

@facebook-github-bot

This comment has been minimized.

Copy link

commented Jun 21, 2019

@jackm321 merged this pull request in 5478d87.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.