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

Support for libtorch-1.3 #202

Merged
merged 18 commits into from Oct 15, 2019

Conversation

junjihashimoto
Copy link
Member

@junjihashimoto junjihashimoto commented Oct 12, 2019

  • Add Dimname and DimnameList of new data-types
  • Fix compilation-error
  • Add Symbol of new data-type
  • Upload binaries of libtorch-1.3
  • Refine auto generated functions.

@junjihashimoto junjihashimoto changed the title Add Dimname and DimnameList to support libtorch-1.3 Support for libtorch-1.3 Oct 12, 2019
@austinvhuang
Copy link
Member

Thanks for getting this going @junjihashimoto! Let us know if there’s aspects others can help with. We may need to tweak some of the examples and higher level apis depending on how the ffi gets updated.

@junjihashimoto
Copy link
Member Author

@austinvhuang
Thank you for your kind offer. Surprisingly some of the examples and higher level api could compile and test without change. I think I should tweak binding.yaml deciding which ffi-functions for dynamic tensors to make.

@tscholak
Copy link
Member

hi @junjihashimoto! Dimname and DimnameList are libtorch primitives for Named Tensors, are they? I'm actually surprised that Named Tensors have been implemented in libtorch. I was almost sure that it was implemented as a wrapper in Python. Cool!

@junjihashimoto
Copy link
Member Author

@tscholak
Yes. The usage is here.
https://github.com/pytorch/pytorch/blob/master/aten/src/ATen/test/NamedTensor_test.cpp
I'm looking forward to seeing it on high level api.

@austinvhuang
Copy link
Member

@junjihashimoto @tscholak from what I understand the trajectory for libtorch is basically feature parity with the python api, which is great in terms of having as much functionality as possible.

There might be some features where we'll have to make design decisions in terms of how they're interfaced though (or even to have high level interfaces at all). For namedtensor, it's not obvious to me what it looks like yet. But we can go ahead with the codegen bindings first and consider options for the functionality.

@tscholak
Copy link
Member

@austinvhuang Oh, yes, for sure we can go ahead without figuring out named tensors! I was just surprised to find any evidence of named tensors in libtorch.

@junjihashimoto junjihashimoto marked this pull request as ready for review October 14, 2019 18:47
@junjihashimoto
Copy link
Member Author

@tscholak @austinvhuang
All tasks for upgrading libtorch are done.
I've found tensor-factory of named-tensor does not work. I don't know the cause.
But existing examples are working, for now I think this pr can be merge.

@tscholak
Copy link
Member

sweet! thanks @junjihashimoto, what an achievement!
I'm not the right person to review the codegen part, but I trust in the tests we wrote in Torch.Static.

@@ -25,13 +25,13 @@ spec = do
(shape (asTensor v)) `shouldBe` [3]
print (toDense x)
-- When we call print for sparse tensor, it throws a exception.
(print x) `shouldThrow` anyException
print x -- `shouldThrow` anyException
Copy link
Member

Choose a reason for hiding this comment

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

doesn't throw anymore? did they implement something sparse-related that wasn't implemented before?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. Our understanding is right. They changed the implementation, but I do not find the change.
We can print out sparse-tensor.

@austinvhuang
Copy link
Member

Wow @junjihashimoto this is amazing. Considering how long it took to catch up to the last PTDC release and now it's down to a few days.

Any idea what's going on with the OSX Circle CI build? Should CircleCI just be deprecated entirely?

If CI passes, I'm good with going ahead and merging.

@junjihashimoto
Copy link
Member Author

@austinvhuang
Thank you for reviewing.
We should deprecate circleci.
CircleCI allows ssh (it is useful to debug.), but the resource is no so much.
It is 1x concurrency, 4GB memory and short timeout on macos.
Sometimes, we waste time to wait for CI on macos.

@junjihashimoto junjihashimoto merged commit e051cbf into hasktorch:master Oct 15, 2019
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.

None yet

3 participants