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

[CI] Test with Poplar 3.3 #45

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

[CI] Test with Poplar 3.3 #45

wants to merge 4 commits into from

Conversation

giordano
Copy link
Collaborator

@giordano giordano commented Mar 14, 2024

Clang.jl is currently not compatible with Julia v1.11, so we can't test this at the moment. Fixing #18 would let us reduce the number of dependencies, including Clang.jl itself (it's only needed to generate the wrappers, not at runtime).

@giordano giordano marked this pull request as draft March 14, 2024 23:29
@giordano giordano force-pushed the mg/poplar-3.3 branch 3 times, most recently from 157ea0e to 7aa4f39 Compare April 9, 2024 00:42
@giordano
Copy link
Collaborator Author

giordano commented Apr 9, 2024

With #62 we now have a version of Clang.jl compatible with Julia v1.11, but building the bindings is failing because the Poplar header files can't be found. The problem is that instead of explicitly setting the environment variables like CPATH and LD_LIBRARY_PATH, in the graphcore/poplar:3.3.0 container they now set BASH_ENV to run a script which sets those variables, compare graphcore/poplar:3.2.0-ubuntu-20.04-20230314 and graphcore/poplar:3.3.0-ubuntu-20.04-20230703. I'm trying to figure out how to make CPATH and LD_LIBRARY_PATH persist throughout the job.

For what is worth, building of the package works locally for me inside

docker run -ti --rm graphcore/poplar:3.3.0-ubuntu-20.04-20230703 bash

so there's hope this will eventually work, but the problem is that here we aren't going through the exact same path as we aren't using the bash entrypoint.

@giordano giordano force-pushed the mg/poplar-3.3 branch 5 times, most recently from ecd2d8e to 0728e50 Compare April 9, 2024 11:08
@giordano
Copy link
Collaborator Author

giordano commented Apr 9, 2024

I believe there's a bug in Clang.jl v0.18.0 with llvm 16, because compilation of the automatically generated wrappers fails here with julia v1.11 with errors like

In file included from /__w/IPUToolkit.jl/IPUToolkit.jl/deps/wrapper/template.cpp:116:
/__w/IPUToolkit.jl/IPUToolkit.jl/deps/wrapper/gen_inline.cpp: In function ‘void define_julia_module(jlcxx::Module&)’:
/__w/IPUToolkit.jl/IPUToolkit.jl/deps/wrapper/gen_inline.cpp:329:41: error: ISO C++ forbids declaration of ‘type name’ with no type [-fpermissive]
  329 | JLOptionFlagsiterator.constructor<const iterator &>();
      |                                         ^~~~~~~~
/__w/IPUToolkit.jl/IPUToolkit.jl/deps/wrapper/gen_inline.cpp:329:23: error: parse error in template argument list
  329 | JLOptionFlagsiterator.constructor<const iterator &>();
      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/__w/IPUToolkit.jl/IPUToolkit.jl/deps/wrapper/gen_inline.cpp:329:53: error: no matching function for call to ‘jlcxx::TypeWrapper<poplar::OptionFlags::iterator>::constructor<<expression error> >()’
  329 | JLOptionFlagsiterator.constructor<const iterator &>();
      |                                                     ^

while it works fine in #48 with the same version of Clang.jl but a different version of Julia/llvm (there we are using julia v1.7/llvm 12). I'll need to compare the generate wrappers with the two different versions of Julia/llvm. Edit: issue opened at JuliaInterop/Clang.jl#482.

@giordano giordano mentioned this pull request Aug 18, 2024
2 tasks
@Gnimuc
Copy link
Contributor

Gnimuc commented Aug 18, 2024

The tricky part is that we need to upgrade both polar (3.2->3.3) and the wrapper generator script, probably CxxWrap as well.

@giordano
Copy link
Collaborator Author

Yes, each upgrade of this package is tricky 🥲 CxxWrap was upgraded in #66, I probably need to rebase this branch on main

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.

2 participants