search

cctbx / cctbx_project

Computational Crystallography Toolbox
https://cci.lbl.gov/docs/cctbx
Other
207 stars 111 forks source link

howto build annlib_adaptbx with annlib #770

Open picca opened 1 year ago

picca commented 1 year ago

Hello, I am preparing a cctbx package with annlib_adaptbx embeded.

My question is how can I build this adaptbx without the annlib which is already available at the system level (package libaa-dev).

thanks for considring.

Fred

picca commented 1 year ago

There is another issue :), the library generated is called ann. but this is a modified version of ann augmented with the ann_adaptor.cpp file. Is it possible to rename this library ann_adaptor in order to avoid a clash when installing the library on the system.

bkpoon commented 1 year ago

One issue is that our annlib is modified to also include self in the list. @nksauter would know more.

From an earlier issue (#460) are you trying to build a Debian package for cctbx? If you point me to your scripts for building, I can help get that sorted out.

picca commented 1 year ago

Yes, here the package which does not work fully for now.

https://tracker.debian.org/pkg/cctbx the repository for the packaging is here.

https://salsa.debian.org/science-team/cctbx

the objectif is to have dials in Debian at the end.

Cheers

bkpoon commented 1 year ago

I am preparing for the ACA meeting, so I will not have time to look into this until I am back (around 8/8). Is it possible for me to fork the repository into my gitlab.com organization and make a merge request into the Debian GitLab repository? Or would I have to run the build locally? It looks like you only have the contents of cctbx_project which does not have all the source. There are additional modules that are added to the source tarball in the releases. I track them in https://github.com/cctbx/cctbx. You do not need all of them, though.

picca commented 1 year ago

Hello

I am preparing for the ACA meeting, so I will not have time to look into this until I am back (around 8/8). Is it possible for me to fork the repository into my gitlab.com organization and make a merge request into the Debian GitLab >repository? Or would I have to run the build locally?

I do not know about this. you can try to do a fork on gitlab and see if the CI is working. You can also create an account on salsa.debian.org (it is open to everyone). Then it sould work out of the box :)

It looks like you only have the contents of cctbx_project which does not have all the source. There are additional modules that are added to the source tarball in the releases. I track them in https://github.com/cctbx/cctbx. You do not need all of them, though.

Yes I do not know where are the official cctbx repository. the one I can build a working cctbx out of the box. (In my dream a pip install would be fantastic ;)

Cheers

picca commented 1 year ago

Hello, I have a question about the annlib test, I try to run it but I do not get the same hash that the one expected.

when I run the sample test it gives

B619FC6D18D5F78F5A55A2D06AD0B03F != E486456DC3A225C40FE8A3A9D9A760E9

All the distance are equals to zero it seems to me correct. the query is just the truncated data file.

I would like to understand what is going on, do you run this test regularly when building cctbx ?, on which platform etc... I am woindering if the StringIO is identical on linux and windows etc...

thanks for your help.

Frederic

bkpoon commented 1 year ago

I have a build running on salsa.debian.org. Can you tell me where to look for the output to this test? Should I make a PR and then you can tell me where the issues are?

The test is checking the hash to the output from the test. You can try printing the output of exercise_nearest_neighbor() to see if there is a difference with the output from my machine (excercise_nearest_neighbor.txt). I generate the output by adding

with open('excercise_nearest_neighbor.txt', 'w') as f:
  f.write(excercise_nearest_neighbor())

to annlib_adaptbx/annlib_adaptbx/sample.py before the assert.

picca commented 1 year ago

Hello, I found the issue

the ann library packages with Debian is the one from this source where ANN_ALLOW_SELF_MATCH is ANNtrue.

https://www.cs.umd.edu/~mount/ANN/

your version of annlib is an older version in which the variable was set to ANNfalse. This why I have this issue...

https://github.com/cctbx/annlib/blob/master/include/ANN/ANN.h#L234

the logic is inverted...

nksauter commented 1 year ago

We compile it both ways for the Python wrapper.

Python class AnnAdaptor uses ANN_ALLOW_SELF_MATCH=ANNfalse

Python class AnnAdaptorSelfInclude uses ANN_ALLOW_SELF_MATCH=ANNtrue

Nick Sauter

On Wed, Oct 12, 2022 at 8:26 AM picca @.***> wrote:

Hello, I found the issue

the ann library packages with Debian is the one from this source where ANN_ALLOW_SELF_MATCH is ANNtrue.

https://www.cs.umd.edu/~mount/ANN/

your version of annlib is an older version in which the variable was set to ANNfalse. This why I have this issue...

https://github.com/cctbx/annlib/blob/master/include/ANN/ANN.h#L234

the logic is inverted...

— Reply to this email directly, view it on GitHub https://github.com/cctbx/cctbx_project/issues/770#issuecomment-1276362659, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADQ24VVE5SMO43VDTB3ZTXDWC3KBLANCNFSM53XXZVUQ . You are receiving this because you were mentioned.Message ID: @.***>

picca commented 1 year ago

Ok, I rebuilt an ann Debian package with your default. I can confirm that all test pass now :). It solved also the dials indexing tests. so to solve this issue for real, I need to upload an ann library into Debian with your defaults. The ann library should not be modified. Maybe the best solution is to create a dedicated ann-cctbx library with both symbols instead.

picca commented 1 year ago

In order to avoid a symbol collision with the current ann library, I will create a ANNSELF_INCLUDE and ANNSELF_EXCLUDE name space and patch the annlib code. this way all symbols used by cctbx will be specific to cctbx.

let's try it...

picca commented 1 year ago

Done, now there is libann-cctbx-dev and libann-cctbx0 packages in order to build cctbx. I patched the code in order to link against ann-cctbx instead of ann and replace all the ANN objects with annself_exclude::ANN.

all test passed now. I think that the 2022.9+ds2+~3.11.2+ds1-2 will be the first version which works for our users :)

thanks for your help.