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

PWGEM:LMee: Adding new KFParticle variables to TreeMaker #22449

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jjungIKF
Copy link
Contributor

@jjungIKF jjungIKF commented Jul 2, 2023

No description provided.

@alibuild
Copy link
Collaborator

alibuild commented Jul 2, 2023

e902453: approval required: 1 of @atoia (Alberica Toia), @chiarazampolli (Chiara Zampolli), @davidrohr (David Rohr), @dsekihat (Daiki Sekihata), @hmurakam (Hikari Murakami), @iarsene (Ionut Cristian Arsene), @jgrosseo (Jan Fiete Grosse-Oetringhaus), @ktf (Giulio Eulisse), @otonvd (Oton Vazquez Doce), @pzhristov (Peter Hristov), @qgp (Jochen Klein), @rbailhac (Raphaelle Bailhache), @sawenzel (Sandro Christian Wenzel), @shahor02 (Ruben Shahoyan), @tkgunji (Taku Gunji), @wiechula (Jens Wiechula)

Comment with +1 to approve and allow automatic merging,or with +test to run tests only. Please comment on the pull request: click here and comment at the bottom of the page.

@dsekihat
Copy link
Contributor

dsekihat commented Jul 2, 2023

+1

@alibuild
Copy link
Collaborator

alibuild commented Jul 2, 2023

e902453: approved: will be automatically merged on successful tests

@alibuild
Copy link
Collaborator

alibuild commented Jul 2, 2023

Error while checking build/AliPhysics/root6 for e902453 at 2023-07-05 18:28:

## sw/BUILD/AliRoot-latest/log
100% tests passed, 0 tests failed out of 101


## sw/BUILD/AliPhysics-latest/log
115/116 Test  #68: load_library_PWGDQdielectron .....................***Failed   38.92 sec
116/116 Test  #70: load_library_PWGDQreducedTree ....................***Failed   38.80 sec
98% tests passed, 2 tests failed out of 116

Full log here.

@jjungIKF
Copy link
Contributor Author

jjungIKF commented Jul 4, 2023

@TimoWilken
Hi Timo, sorry for pinging you. You commented earlier on my old pull request. Unfortunately, I cannot open the link you provided in your post anymore. Could you help me again with this issue? I am currently a bit puzzled as I compile it locally and the tests run without issues.

@TimoWilken
Copy link
Contributor

Hi @jjungIKF!

I was referring to the following lines you added after the new if(KFParticle_FOUND) block in PWGDQ/dielectron/CMakeLists.txt:

set(LIBDEPS ${ALIPHYSICS_DEPENCIES} ${ALIROOT_DEPENDENCIES} ${ROOT_DEPENDENCIES})
generate_rootmap("${MODULE}" "${LIBDEPS}" "${CMAKE_CURRENT_SOURCE_DIR}/${MODULE}LinkDef.h")

In your previous PR, I think you didn't delete the same lines further above (lines 136+137), but now it seems you have.

I'm still a bit confused why you've done this -- doesn't the set(LIBDEPS ${ALIPHYSICS_DEPENCIES} ${ALIROOT_DEPENDENCIES} ${ROOT_DEPENDENCIES}) after the set(LIBDEPS ${LIBDEPS} ${KFPARTICLE_LIBRARY}) wipe that change out again?

Also, in this case, presumably ${KFPARTICLE_LIBRARY} won't be in ${LIBDEPS} on line 174? So why not leave the following two lines both where they were originally, i.e. on lines 136+137, and not duplicate them?

set(LIBDEPS ${ALIPHYSICS_DEPENCIES} ${ALIROOT_DEPENDENCIES} ${ROOT_DEPENDENCIES})
generate_rootmap("${MODULE}" "${LIBDEPS}" "${CMAKE_CURRENT_SOURCE_DIR}/${MODULE}LinkDef.h")

When you compile and run locally, do you specifically run the load_library_PWGDQdielectron and load_library_PWGDQreducedTree unit tests? Those are the cause of the timeout in CI.

Apart from this CMake change I explained above, nothing immediately jumps out at me as a possible cause of the timeout...

@jjungIKF
Copy link
Contributor Author

jjungIKF commented Jul 4, 2023

Hi @jjungIKF!

I was referring to the following lines you added after the new if(KFParticle_FOUND) block in PWGDQ/dielectron/CMakeLists.txt:

set(LIBDEPS ${ALIPHYSICS_DEPENCIES} ${ALIROOT_DEPENDENCIES} ${ROOT_DEPENDENCIES})
generate_rootmap("${MODULE}" "${LIBDEPS}" "${CMAKE_CURRENT_SOURCE_DIR}/${MODULE}LinkDef.h")

In your previous PR, I think you didn't delete the same lines further above (lines 136+137), but now it seems you have.

I'm still a bit confused why you've done this -- doesn't the set(LIBDEPS ${ALIPHYSICS_DEPENCIES} ${ALIROOT_DEPENDENCIES} ${ROOT_DEPENDENCIES}) after the set(LIBDEPS ${LIBDEPS} ${KFPARTICLE_LIBRARY}) wipe that change out again?

Also, in this case, presumably ${KFPARTICLE_LIBRARY} won't be in ${LIBDEPS} on line 174? So why not leave the following two lines both where they were originally, i.e. on lines 136+137, and not duplicate them?

set(LIBDEPS ${ALIPHYSICS_DEPENCIES} ${ALIROOT_DEPENDENCIES} ${ROOT_DEPENDENCIES})
generate_rootmap("${MODULE}" "${LIBDEPS}" "${CMAKE_CURRENT_SOURCE_DIR}/${MODULE}LinkDef.h")

When you compile and run locally, do you specifically run the load_library_PWGDQdielectron and load_library_PWGDQreducedTree unit tests? Those are the cause of the timeout in CI.

Apart from this CMake change I explained above, nothing immediately jumps out at me as a possible cause of the timeout...

Thank you for the fast reply @TimoWilken
To be completely honest, I had to copy these lines from CMakefile in vertexingHF directory in order to make it compile on my local machine. I implemented this change as I want to use the root implementation of the KFParticle instead of the one in AliRoot. I am not too familiar with how CMakefiles actually work and hoped that the same code would fix my problem as it did on my local machine.
Regarding the unit test. I have to check the next time I build AliPhysics again. I do not specifically request any unit test but only run the test which are done with the default build command.

@TimoWilken TimoWilken marked this pull request as draft July 5, 2023 16:25
@TimoWilken
Copy link
Contributor

Hi @jjungIKF, I've marked this PR as draft to avoid long delays for other PRs. Let me know if those unit tests work locally for you!

@jjungIKF
Copy link
Contributor Author

jjungIKF commented Jul 6, 2023

Hi @jjungIKF, I've marked this PR as draft to avoid long delays for other PRs. Let me know if those unit tests work locally for you!

Hi @TimoWilken,
so I had another look at it and both unit test go through without any issues and the time passed for the test seems about normal as well.

@jjungIKF
Copy link
Contributor Author

@TimoWilken
Do you have any other ideas on what i could check?

@pzhristov
Copy link
Contributor

+test

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.

5 participants