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

Update wrapper #54

Closed
stemann opened this issue Aug 26, 2022 · 13 comments · Fixed by #64
Closed

Update wrapper #54

stemann opened this issue Aug 26, 2022 · 13 comments · Fixed by #64
Assignees

Comments

@stemann
Copy link
Collaborator

stemann commented Aug 26, 2022

@DhairyaLGandhi Can you help with some hints for how to most easily update the wrapper (e.g. for Torch 1.10) ?

It seems there has been a change on the return type from void to int - is there some ocaml generator code somewhere?

I've compared the contents of c37c480 and https://github.com/LaurentMazare/ocaml-torch, and found that the original source for the wrapper seems to have been tag 0.8, i.e. https://github.com/LaurentMazare/ocaml-torch/tree/85da09301df133e9d97a8340c1ce74a13651ca60/src/wrapper

Re-applying the complete set of changes to the build dir. (e.g. stemann@e86f40e) on top of an updated ocaml-torch copy (https://github.com/LaurentMazare/ocaml-torch/tree/a6499811f40282a071179d4306afbbb6023dcc4a/src/wrapper) is not completely problem-free.

Relates to JuliaPackaging/Yggdrasil#4554

@DhairyaLGandhi
Copy link
Member

Absolutely! The cmake Script does need to be updated. Could you say what the issue specifically was? Happy to help get this updated

@stemann
Copy link
Collaborator Author

stemann commented Aug 26, 2022

OK.

I just tried simply cherry picking stemann@e86f40e on top of https://github.com/IHPSystems/Torch.jl/tree/stemann/wrap_torch_1.10 and ran into quite a lot of merge conflicts. So I thought that there might be a small change to https://github.com/LaurentMazare/ocaml-torch/tree/main/src/gen which could make most of the merge conflicts go away :-)

@stemann
Copy link
Collaborator Author

stemann commented Aug 26, 2022

Making some progress - I've re-done the edits to the gen.ml script needed for outputting the current generated C api: https://github.com/IHPSystems/Torch.jl/tree/stemann/build_wrapper/build/wrapper_generator

@DhairyaLGandhi
Copy link
Member

Why are some functions excluded? Is there a way to build wrappers for these as well? I also noticed that you'd redefined types in ocaml to match cpp

@stemann
Copy link
Collaborator Author

stemann commented Aug 26, 2022

Which functions do you mean ("functions excluded")?

I just redefined types in ocaml to match the torch_api_generated files that are currently in this repo. - to be sure I could replicate the same process with a newer libtorch.

@ToucheSir
Copy link
Member

Presumably "functions excluded" refers to https://github.com/IHPSystems/Torch.jl/blob/stemann/build_wrapper/build/wrapper_generator/bin/main.ml#L8, but my understanding is that list is integral to the build process of ocaml-torch and was implicitly used anyhow for the initial binding generation.

Would it be easier to directly update to the latest binding generator and then try to revamp the Julia API if anything's changed? Also, would using https://github.com/LaurentMazare/tch-rs/tree/main/gen help since it appears to be more up to date than https://github.com/LaurentMazare/ocaml-torch/tree/main/src/gen?

@stemann
Copy link
Collaborator Author

stemann commented Aug 27, 2022

Yes, we should definitely use the up to date tch-rs wrapper generator - just wanted to reproduce the steps for generating 1.4 bindings.

The wrapper generators are identical wrt. generating C code if one compares ocaml-torch for torch 1.10 with tch-rs for torch 1.10.

@stemann
Copy link
Collaborator Author

stemann commented Oct 2, 2022

Planned course of action:

  1. Pin Torch_jll dependency to v1.4 (Limit compat of Torch_jll to v1.4 #55)
  2. Build/Register Torch_jll v1.10.2+ (just libtorch - without wrapper)
  1. Build/Register TorchWrapper_jll - which builds/depends on Torch_jll
  2. Update Torch.jl to use TorchWrapper_jll

@stemann
Copy link
Collaborator Author

stemann commented Aug 2, 2023

Finally got around to doing something more wrt. bullet point 4 - cf. #56.

@stemann
Copy link
Collaborator Author

stemann commented Nov 11, 2023

Updated plan:

  1. Fixed compat to enable tests on CI #60
  2. Updated Julia wrapper generator #59 to have that part cleaned-up
  3. Updated C wrapper wrt. Torch v1.10 #61
  4. Update Torch_jll recipe in Yggdrasil - it is missing CUDA platform augmentation, I believe. Seems like this was already done.
  5. Added TorchCAPI recipe JuliaPackaging/Yggdrasil#6004 : Build the C wrapper in Yggdrasil, e.g. as TorchCAPI_jll - having Torch_jll as a dependency.
  6. Updated for Torch v1.10.2, and NNlib >= v0.8 #64: Update Torch.jl to use the updated C wrapper

@rayegun
Copy link

rayegun commented Apr 14, 2024

@stemann what can I do to get this through the finish line? Also we'd probably like to go with a recent 2.x release here soon

@stemann
Copy link
Collaborator Author

stemann commented Apr 15, 2024

This reminder helps a lot :-)

But push for and remind me about #61 - and help review it - should help.

2.x should be done by first updating Torch_jll and its dependencies - cf. T/Torch/deps.md in Yggdrasil.

@stemann
Copy link
Collaborator Author

stemann commented Aug 8, 2024

With #61 now merged, all should be set to get points 4 and 5 completed 🚀

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 a pull request may close this issue.

4 participants