Skip to content

Fix rpaths #18

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jun 7, 2022
Merged

Fix rpaths #18

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7d3c0c3
Fix rpaths
Jun 3, 2022
ce6e14f
Bump build number
Jun 3, 2022
61dc6e2
MNT: Re-rendered with conda-build 3.21.9, conda-smithy 3.20.0, and co…
Jun 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .circleci/config.yml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion recipe/meta.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,13 @@ source:
sha256: 06a0f567c1b82746841424d63627e311d93cba4ab0e121f7d9d64701e753467a

build:
number: 0
number: 1
string: cuda{{ cuda_compiler_version | replace('.', '') }}py{{ CONDA_PY }}h{{ PKG_HASH }}_{{ PKG_BUILDNUM }} # [cuda_compiler_version != "None"]
string: cpu_py{{ CONDA_PY }}h{{ PKG_HASH }}_{{ PKG_BUILDNUM }} # [cuda_compiler_version == "None"]
skip: true # [win or (linux and cuda_compiler_version in (undefined, 'None', '10.2'))]
rpaths:
- lib/
- {{ SP_DIR }}/torch/lib
Copy link
Contributor

Choose a reason for hiding this comment

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

can we try adding:

requirements:
   run:
        - cudnn                           # [cuda_compiler_version != "None"]
        - nccl                            # [cuda_compiler_version != "None"]

similar to https://github.com/conda-forge/pytorch-cpu-feedstock/blob/main/recipe/meta.yaml#L58 instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess these two lines won't help.

2022-06-01T04:06:06.6901147Z WARNING (openmm-torch,lib/libOpenMMTorch.so): $RPATH/libc10.so not found in packages, sysroot(s) nor the missing_dso_whitelist.
2022-06-01T04:06:06.6901798Z .. is this binary repackaging?
2022-06-01T04:06:06.6916172Z WARNING (openmm-torch,lib/libOpenMMTorch.so): $RPATH/libtorch_cpu.so not found in packages, sysroot(s) nor the missing_dso_whitelist.
2022-06-01T04:06:06.6917017Z .. is this binary repackaging?
2022-06-01T04:06:06.6930804Z WARNING (openmm-torch,lib/libOpenMMTorch.so): $RPATH/libtorch_cuda.so not found in packages, sysroot(s) nor the missing_dso_whitelist.
2022-06-01T04:06:06.6931457Z .. is this binary repackaging?
2022-06-01T04:06:06.6945306Z WARNING (openmm-torch,lib/libOpenMMTorch.so): $RPATH/libtorch.so not found in packages, sysroot(s) nor the missing_dso_whitelist.
2022-06-01T04:06:06.6945926Z .. is this binary repackaging?

Is a really strange error to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Truthfully, this is where my knowledge of the build systems ends.

It is unclear to me why we need the rpaths here, and not in torchvision.

https://github.com/conda-forge/torchvision-feedstock/blob/main/recipe/meta.yaml#L27

@conda-forge/cudatoolkit-dev if you could chime in here it owuld be helpful.

Copy link
Contributor

Choose a reason for hiding this comment

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

One reason why I think this is happening is that you are doing alot of custom configuration to find the file libtoch.so within the python sitepackages directory. This is somewhat frowned upon and is not OK. this is likely what all these RPATHs are trying to warn you about. I think, that for now, you should do what you need to do to link to this package. to "work". I think adding these "IGNORE RPATHS" is ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can we try adding:

requirements:
   run:
        - cudnn                           # [cuda_compiler_version != "None"]
        - nccl                            # [cuda_compiler_version != "None"]

similar to https://github.com/conda-forge/pytorch-cpu-feedstock/blob/main/recipe/meta.yaml#L58 instead?

Why?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think adding the RPATH is correct. My longer term goal is to package libtorch to enable this kind of stuff to happen naturally.

The hackery i'm talking about is:
https://github.com/conda-forge/openmm-torch-feedstock/blob/main/recipe/build.sh#L12

image

Setting TORCH_DIR to this strange directory is the hack.

Though for now, I don't have a better solution. My latest attempts at refactoring the pytorch package are documented in conda-forge/pytorch-cpu-feedstock#108 but there is still a long way to go (especially with GPU) and with the python side of the package too.

Copy link
Contributor

Choose a reason for hiding this comment

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

My longer term goal is to package libtorch to enable this kind of stuff to happen naturally.

A straightforward way of doing that would be to have it put its libraries in the main lib directory instead of a private lib directory inside the Python module.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you @peastman. Given the constraints of time, I don't have time to do this at the moment. But yes, it is in the longer term goal i outlined in conda-forge/pytorch-cpu-feedstock#108

Copy link
Contributor

Choose a reason for hiding this comment

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

So given the current state of things, I think we should be good to merge this in, I see how it is ideal but it seems there isn't really anything else to do until pytorch drops the libs in the correct place (or a separate package is created)

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, the only one i'm still unconvinced of is the lib RPATH. Not sure where that is coming from. But if you all want to merge, then I don't see "harm" that can be made from this.


requirements:
build:
Expand Down