Skip to content

Replace the Query option by Filter and Aliphysics ones #63

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

Closed
wants to merge 7 commits into from
Closed

Replace the Query option by Filter and Aliphysics ones #63

wants to merge 7 commits into from

Conversation

aphecetche
Copy link
Contributor

With those modifications, one can trigger a stage+filter operation on an Alice Analysis Facility

PS: this is my first ever pull request (being Root or otherwise) so please bear with me if that's not the correct procedure to contribute back to Root...

@gganis
Copy link
Contributor

gganis commented May 5, 2015

Hi,

Thanks for the proposal but these modifications would break compatibility with VAF users.
Please get in contact with Dario Berzano about this class and agree with him on any change before submitting a new request.

Cheers, GG

@dberzano
Copy link
Contributor

dberzano commented May 5, 2015

Hello @aphecetche,

replacing an existing keyword with another one may break compatibility with existing users. Even if Filter sounds more appropriate, I would avoid doing that.

Pushing the concepts of AliRoot and AliPhysics into ROOT is wrong. This is ALICE stuff and should not get "too much" into ROOT's code.

"too much" = the AliEn interface was inside ROOT already (wrongly, IMHO), and this dataset manager has been added to ROOT solely because it requires AliEn only to work.

To me those patches reflect your quite specific use case. What is it you'd like to achieve, and that cannot be done with the tools you have?

Cheers

d.

@aphecetche
Copy link
Contributor Author

Hi,

OK. Yes, this is highly dependent on AliRoot/AliPhysics.

What is currently in production in the SAF is a procedure to get a filtering be performed at the same time data is being staged (*). I’ve deployed on SAF my own root version with those changes and I’m happy with the way it work, but it’s really awkward for the user because they cannot use the same root version for the staging and for their analysis (and they must play with datasetmanager cache of the queries to get the right files…). That’s why I’d want to have it « officially » in Root.

To way to trigger such a filtering is to add bit and pieces to the file names generated by the TDataSetManager, e.g. :

const char* query = "Find;FileName=AliESDs.root;BasePath=/alice/data/2011/LHC11h/000169838/ESDs/pass2_muon;Filter=ESDMUON;Aliphysics=vAN-20150213";

gProof->ShowDataSet(query);
will generate filenames as :

alien:///alice/data/2011/LHC11h/000169838/ESDs/pass2_muon/11000169838080.42/AliESDs.FILTER_ESDMUON_WITH_ALIPHYSICS_vAN-20150213.root alien:///alice/data/2011/LHC11h/000169838/ESDs/pass2_muon/11000169838080.42/AliESDs.FILTER_ESDMUON_WITH_ALIPHYSICS_vAN-20150213.root

On the workers the staging script is deciphering the filename in order to call

$ALICE_PHYSICS/aaf-stage-and-filter --from source_url --to destination_url --filter filtername

where :
ALICE_PHYSICS points to (cvmfs version of) vAN-20150213,
source_url is alien:///alice/data/2011/LHC11h/000169838/ESDs/pass2_muon/11000169838080.42/AliESDs.root alien:///alice/data/2011/LHC11h/000169838/ESDs/pass2_muon/11000169838080.42/AliESDs.root
filter name is ESDMUON

Hope this clarifies the intent (more bla at http://aphecetche.github.io/aafu/doc/#Datafiltering http://aphecetche.github.io/aafu/doc/#Datafiltering).

If the dataset manager can be easily provided to users without having it in Root, I’m all for it.

Regards,

Le 5 mai 2015 à 18:40, Dario Berzano [email protected] a écrit :

Hello @aphecetche https://github.com/aphecetche,

replacing an existing keyword with another one may break compatibility with existing users. Even if Filter sounds more appropriate, I would avoid doing that.

Pushing the concepts of AliRoot and AliPhysics into ROOT is wrong. This is ALICE stuff and should not get "too much" into ROOT's code.

"too much" = the AliEn interface was inside ROOT already (wrongly, IMHO), and this dataset manager has been added to ROOT solely because it requires AliEn only to work.

To me those patches reflect your quite specific use case. What is it you'd like to achieve, and that cannot be done with the tools you have?

Cheers

d.


Reply to this email directly or view it on GitHub #63 (comment).

@dberzano
Copy link
Contributor

dberzano commented May 6, 2015

Ok, so to sum up.

User requests the following file:

alien://path/AliESDs.FILTER_ESDMUON_WITH_ALIPHYSICS_vAN-20150213.root

This is not an actual file: it contains a special string that tells the dataset stager (afdsmgrd: or more precisely, the download script it invokes for each file) to:

  • Take the source file alien://path/AliESDs.root from AliEn
  • Apply filtering of type ESDMUON
  • Use AliPhysics vAN-20150213 for that purpose
  • Stage a file called root://path/AliESDs.FILTER_ESDMUON_WITH_ALIPHYSICS_vAN-20150213.root on your local cluster

(Please correct me if I am wrong.)

I remember that some time ago we have agreed on having 34bb2e2 addressing the problem: Query=<string> was used to append an arbitrary ?<string> to the generated filename. For instance:

Find;FileName=AliESDs.root;BasePath=/alice/data/2011/LHC11h/000169838/ESDs/pass2_muon;Query=ESDMUON_vAN-20150213

would generate filenames in the form:

proto://path/root_archive.zip?ESDMUON_vAN-20150213#AliESDs.root

Question: did this approach work for you (again, see this: 34bb2e2)? If it did not, why? If it is just a matter of typing less awkward strings for an extremely custom use case, then your patch cannot be accepted and I suggest replacing it with some good documentation for the end user :-)

As an alternative, you can provide them with a macro that generates the Query= part for you, if it is less awkward for them.

Ah, I also see that between your patches there is an option to "stat" the file if filtered. I can see the point there: stating a file which is not there would trigger a download on a specifically configured xrootd storage. This specific patch cannot be accepted by any means as it goes against a concept we have been trying to establish since a long time: ROOT's dataset manager is about managing lists of files. Staging files is a job that must be done elsewhere, for instance by a storage system or by a mechanism interacting with it.

Thing is, we let data management in ROOT out of the door for a reason, and we don't want it to come in again through the window :-)

Cheers

d.

@aphecetche
Copy link
Contributor Author

Le 6 mai 2015 à 09:54, Dario Berzano [email protected] a écrit :

Ok, so to sum up.

User requests the following file:

alien://path/AliESDs.FILTER_ESDMUON_WITH_ALIPHYSICS_vAN-20150213.root
This is not an actual file: it contains a special string that tells the dataset stager (afdsmgrd: or more precisely, the download script it invokes for each file) to:

Take the source file alien://path/AliESDs.root from AliEn
Apply filtering of type ESDMUON
Use AliPhysics vAN-20150213 for that purpose
Stage a file called root://path/AliESDs.FILTER_ESDMUON_WITH_ALIPHYSICS_vAN-20150213.root on your local cluster
(Please correct me if I am wrong.)

No need for correction, you got it right.
I remember that some time ago we have agreed on having 34bb2e2 34bb2e2 addressing the problem: Query= was used to append an arbitrary ? to the generated filename. For instance:

Find;FileName=AliESDs.root;BasePath=/alice/data/2011/LHC11h/000169838/ESDs/pass2_muon;Query=ESDMUON_vAN-20150213
would generate filenames in the form:

proto://path/root_archive.zip?ESDMUON_vAN-20150213#AliESDs.root proto://path/root_archive.zip?ESDMUON_vAN-20150213#AliESDs.root

I did try and sent you a mail a while back (15/09/2014), which was :

Hi Dario,

With lots of delay I'm finally testing this part and … encountered a problem. (we are migrating SAF1 to SAF2 this week and there are a number of filtered datasets that I’d like to be able to put back on SAF2 in a not so distant future… (*))

It seems the Query= is honored only in the case of FileName=root_archive.zip

e.g. the following query returns just like the one without Query=

gProof->ShowDataSet("Find;BasePath=/alice/data/2011/LHC11h/000170593/ESDs/pass2_muon/AOD119;FileName=AliAOD.Muons.root;Query=toto »)

Is that on purpose ?

Thanks,

(*) for instance :

Find;BasePath=/alice/data/2013/LHC13c/000195644/ESDs/pass2/AOD139;FileName=AliAOD.root is 418 GB on Grid, and ~10 GB filtered on SAF1…

Question: did this approach work for you (again, see this: 34bb2e2 34bb2e2)? If it did not, why? If it is just a matter of typing less awkward strings for an extremely custom use case, then your patch cannot be accepted and I suggest replacing it with some good documentation for the end user :-)

So no, the approach as it is does not work for us as :

a) we rarely stage full archives (that’s for sure)
b) out of my head right now I cannot find back why but I kind of remember the proto://xxx?filter_aliversion#…. syntax did cause an issue somewhere

As an alternative, you can provide them with a macro that generates the Query= part for you, if it is less awkward for them.

Ah, I also see that between your patches there is an option to "stat" the file if filtered. I can see the point there: stating a file which is not there would trigger a download on a specifically configured xrootd storage. This specific patch cannot be accepted by any means as it goes against a concept we have been trying to establish since a long time: ROOT's dataset manager is about managing lists of files. Staging files is a job that must be done elsewhere, for instance by a storage system or by a mechanism interacting with it.

Well, the intent of this part of the patch was to get a correct feed back (for the user) about the actual size of the file (as the size in the collection is the alien catalog size, i.e. before filtering). What you say here is that I cannot do that with a stat as this would trigger a download (in case the file is not there), do I get it correctly ? Any idea on how to achieve what I want ?
Thing is, we let data management in ROOT out of the door for a reason, and we don't want it to come in again through the window :-)

Well, yes, except that users do need data management at some point…

One other way maybe : is there a way to get a custom dataset manager (I feel I already asked this question at some point…) ?

Cheers

d.


Reply to this email directly or view it on GitHub #63 (comment).

@dberzano
Copy link
Contributor

dberzano commented May 6, 2015

Ok right so we have a bug here :-)

Can you please open a ticket (on the ALICE JIRA, not the ROOT one) and assign it to me, just saying that the query string is not honored in case you don't use the archive? If you can provide a minimal patch fixing it that'd be great :-)

(Emails get lost, as you have seen: tickets do not!)

Concerning the stat, it seems that I have got the stat part wrong: this part actually makes sense. Can you also open a separate ticket (also on ALICE JIRA) on that? No need to send any patch as you have already done it :-)

A custom dataset manager is possible only tampering with ROOT code (I guess it's a "no" in your case).

Cheers,

d.

@Axel-Naumann
Copy link
Member

Closing as per Gerri's request.

ellert added a commit to ellert/root that referenced this pull request Jan 28, 2025
 526/1416 Test   root-project#63: pyunittests-bindings-pyroot-pythonizations-pyroot-pyz-tf-pycallables ..........................***Failed   75.51 sec
test_callable (tf_pycallables.TF1.test_callable)
Test function provided as callable ... /usr/include/c++/15/bits/stl_bvector.h:1134: std::vector<bool, _Alloc>::reference std::vector<bool, _Alloc>::operator[](size_type) [with _Alloc = std::allocator<bool>; reference = std::vector<bool>::reference; size_type = long unsigned int]: Assertion '__n < this->size()' failed.
 *** Break *** abort

===========================================================
The lines below might hint at the cause of the crash. If you see question
marks as part of the stack trace, try to recompile with debugging information
enabled and export CLING_DEBUG=1 environment variable before running.
You may get help by asking at the ROOT forum https://root.cern/forum
preferably using the command (.forum bug) in the ROOT prompt.
Only if you are really convinced it is a bug in ROOT then please submit a
report at https://root.cern/bugs or (preferably) using the command (.gh bug) in
the ROOT prompt. Please post the ENTIRE stack trace
from above as an attachment in addition to anything else
that might help us fixing this issue.
===========================================================
 root-project#10 0x00007f6bcac7fea4 in __pthread_kill_implementation () from /lib64/libc.so.6
 root-project#11 0x00007f6bcac264de in raise () from /lib64/libc.so.6
 root-project#12 0x00007f6bcac0e350 in abort () from /lib64/libc.so.6
 root-project#13 0x00007f6bc9a0be02 in std::__glibcxx_assert_fail(char const*, int, char const*, char const*) () from /lib64/libstdc++.so.6
 root-project#14 0x00007f6bb9b15257 in std::vector<bool, std::allocator<bool> >::operator[](unsigned long) [clone .part.0] [clone .lto_priv.0] (__n=<optimized out>, this=<optimized out>) at /usr/include/c++/15/bits/stl_bvector.h:1134
 root-project#15 0x00007f6bb9bab976 in std::vector<bool, std::allocator<bool> >::operator[] (this=<synthetic pointer>, __n=0) at /usr/include/c++/15/bits/stl_bvector.h:201
 root-project#16 CPyCppyy::Utility::ConstructCallbackPreamble (retType="Double_t", argtypes=..., code=Python Exception <class 'IndexError'>: list index out of range
 root-project#17 0x00007f6bb9b389e0 in PyFunction_AsCPointer (pyobject=<optimized out>, pyobject
entry=0x7f6bb8f838c0, rettype="Double_t", signature="(Double_t*,Double_t*)") at /builddir/build/BUILD/root-6.34.02-build/root-6.34.02/bindings/pyroot/cppyy/CPyCppyy/src/Converters.cxx:2713
 root-project#18 0x00007f6bb9b39851 in CPyCppyy::(anonymous namespace)::FunctionPointerConverter::SetArg (this=0x55eb25d61d40, pyobject=0x7f6bb8f838c0, para=..., ctxt=0x7ffc484855c0) at /builddir/build/BUILD/root-6.34.02-build/root-6.34.02/bindings/pyroot/cppyy/CPyCppyy/src/Converters.cxx:2768
guitargeek pushed a commit that referenced this pull request Jan 31, 2025
 526/1416 Test   #63: pyunittests-bindings-pyroot-pythonizations-pyroot-pyz-tf-pycallables ..........................***Failed   75.51 sec
test_callable (tf_pycallables.TF1.test_callable)
Test function provided as callable ... /usr/include/c++/15/bits/stl_bvector.h:1134: std::vector<bool, _Alloc>::reference std::vector<bool, _Alloc>::operator[](size_type) [with _Alloc = std::allocator<bool>; reference = std::vector<bool>::reference; size_type = long unsigned int]: Assertion '__n < this->size()' failed.
 *** Break *** abort

===========================================================
The lines below might hint at the cause of the crash. If you see question
marks as part of the stack trace, try to recompile with debugging information
enabled and export CLING_DEBUG=1 environment variable before running.
You may get help by asking at the ROOT forum https://root.cern/forum
preferably using the command (.forum bug) in the ROOT prompt.
Only if you are really convinced it is a bug in ROOT then please submit a
report at https://root.cern/bugs or (preferably) using the command (.gh bug) in
the ROOT prompt. Please post the ENTIRE stack trace
from above as an attachment in addition to anything else
that might help us fixing this issue.
===========================================================
 #10 0x00007f6bcac7fea4 in __pthread_kill_implementation () from /lib64/libc.so.6
 #11 0x00007f6bcac264de in raise () from /lib64/libc.so.6
 #12 0x00007f6bcac0e350 in abort () from /lib64/libc.so.6
 #13 0x00007f6bc9a0be02 in std::__glibcxx_assert_fail(char const*, int, char const*, char const*) () from /lib64/libstdc++.so.6
 #14 0x00007f6bb9b15257 in std::vector<bool, std::allocator<bool> >::operator[](unsigned long) [clone .part.0] [clone .lto_priv.0] (__n=<optimized out>, this=<optimized out>) at /usr/include/c++/15/bits/stl_bvector.h:1134
 #15 0x00007f6bb9bab976 in std::vector<bool, std::allocator<bool> >::operator[] (this=<synthetic pointer>, __n=0) at /usr/include/c++/15/bits/stl_bvector.h:201
 #16 CPyCppyy::Utility::ConstructCallbackPreamble (retType="Double_t", argtypes=..., code=Python Exception <class 'IndexError'>: list index out of range
 #17 0x00007f6bb9b389e0 in PyFunction_AsCPointer (pyobject=<optimized out>, pyobject
entry=0x7f6bb8f838c0, rettype="Double_t", signature="(Double_t*,Double_t*)") at /builddir/build/BUILD/root-6.34.02-build/root-6.34.02/bindings/pyroot/cppyy/CPyCppyy/src/Converters.cxx:2713
 #18 0x00007f6bb9b39851 in CPyCppyy::(anonymous namespace)::FunctionPointerConverter::SetArg (this=0x55eb25d61d40, pyobject=0x7f6bb8f838c0, para=..., ctxt=0x7ffc484855c0) at /builddir/build/BUILD/root-6.34.02-build/root-6.34.02/bindings/pyroot/cppyy/CPyCppyy/src/Converters.cxx:2768
hageboeck pushed a commit to hageboeck/root that referenced this pull request Feb 14, 2025
 526/1416 Test   root-project#63: pyunittests-bindings-pyroot-pythonizations-pyroot-pyz-tf-pycallables ..........................***Failed   75.51 sec
test_callable (tf_pycallables.TF1.test_callable)
Test function provided as callable ... /usr/include/c++/15/bits/stl_bvector.h:1134: std::vector<bool, _Alloc>::reference std::vector<bool, _Alloc>::operator[](size_type) [with _Alloc = std::allocator<bool>; reference = std::vector<bool>::reference; size_type = long unsigned int]: Assertion '__n < this->size()' failed.
 *** Break *** abort

===========================================================
The lines below might hint at the cause of the crash. If you see question
marks as part of the stack trace, try to recompile with debugging information
enabled and export CLING_DEBUG=1 environment variable before running.
You may get help by asking at the ROOT forum https://root.cern/forum
preferably using the command (.forum bug) in the ROOT prompt.
Only if you are really convinced it is a bug in ROOT then please submit a
report at https://root.cern/bugs or (preferably) using the command (.gh bug) in
the ROOT prompt. Please post the ENTIRE stack trace
from above as an attachment in addition to anything else
that might help us fixing this issue.
===========================================================
 #10 0x00007f6bcac7fea4 in __pthread_kill_implementation () from /lib64/libc.so.6
 #11 0x00007f6bcac264de in raise () from /lib64/libc.so.6
 #12 0x00007f6bcac0e350 in abort () from /lib64/libc.so.6
 #13 0x00007f6bc9a0be02 in std::__glibcxx_assert_fail(char const*, int, char const*, char const*) () from /lib64/libstdc++.so.6
 root-project#14 0x00007f6bb9b15257 in std::vector<bool, std::allocator<bool> >::operator[](unsigned long) [clone .part.0] [clone .lto_priv.0] (__n=<optimized out>, this=<optimized out>) at /usr/include/c++/15/bits/stl_bvector.h:1134
 root-project#15 0x00007f6bb9bab976 in std::vector<bool, std::allocator<bool> >::operator[] (this=<synthetic pointer>, __n=0) at /usr/include/c++/15/bits/stl_bvector.h:201
 root-project#16 CPyCppyy::Utility::ConstructCallbackPreamble (retType="Double_t", argtypes=..., code=Python Exception <class 'IndexError'>: list index out of range
 root-project#17 0x00007f6bb9b389e0 in PyFunction_AsCPointer (pyobject=<optimized out>, pyobject
entry=0x7f6bb8f838c0, rettype="Double_t", signature="(Double_t*,Double_t*)") at /builddir/build/BUILD/root-6.34.02-build/root-6.34.02/bindings/pyroot/cppyy/CPyCppyy/src/Converters.cxx:2713
 root-project#18 0x00007f6bb9b39851 in CPyCppyy::(anonymous namespace)::FunctionPointerConverter::SetArg (this=0x55eb25d61d40, pyobject=0x7f6bb8f838c0, para=..., ctxt=0x7ffc484855c0) at /builddir/build/BUILD/root-6.34.02-build/root-6.34.02/bindings/pyroot/cppyy/CPyCppyy/src/Converters.cxx:2768

(cherry picked from commit a87b474)
dpiparo pushed a commit that referenced this pull request Feb 18, 2025
 526/1416 Test   #63: pyunittests-bindings-pyroot-pythonizations-pyroot-pyz-tf-pycallables ..........................***Failed   75.51 sec
test_callable (tf_pycallables.TF1.test_callable)
Test function provided as callable ... /usr/include/c++/15/bits/stl_bvector.h:1134: std::vector<bool, _Alloc>::reference std::vector<bool, _Alloc>::operator[](size_type) [with _Alloc = std::allocator<bool>; reference = std::vector<bool>::reference; size_type = long unsigned int]: Assertion '__n < this->size()' failed.
 *** Break *** abort

===========================================================
The lines below might hint at the cause of the crash. If you see question
marks as part of the stack trace, try to recompile with debugging information
enabled and export CLING_DEBUG=1 environment variable before running.
You may get help by asking at the ROOT forum https://root.cern/forum
preferably using the command (.forum bug) in the ROOT prompt.
Only if you are really convinced it is a bug in ROOT then please submit a
report at https://root.cern/bugs or (preferably) using the command (.gh bug) in
the ROOT prompt. Please post the ENTIRE stack trace
from above as an attachment in addition to anything else
that might help us fixing this issue.
===========================================================
 #10 0x00007f6bcac7fea4 in __pthread_kill_implementation () from /lib64/libc.so.6
 #11 0x00007f6bcac264de in raise () from /lib64/libc.so.6
 #12 0x00007f6bcac0e350 in abort () from /lib64/libc.so.6
 #13 0x00007f6bc9a0be02 in std::__glibcxx_assert_fail(char const*, int, char const*, char const*) () from /lib64/libstdc++.so.6
 #14 0x00007f6bb9b15257 in std::vector<bool, std::allocator<bool> >::operator[](unsigned long) [clone .part.0] [clone .lto_priv.0] (__n=<optimized out>, this=<optimized out>) at /usr/include/c++/15/bits/stl_bvector.h:1134
 #15 0x00007f6bb9bab976 in std::vector<bool, std::allocator<bool> >::operator[] (this=<synthetic pointer>, __n=0) at /usr/include/c++/15/bits/stl_bvector.h:201
 #16 CPyCppyy::Utility::ConstructCallbackPreamble (retType="Double_t", argtypes=..., code=Python Exception <class 'IndexError'>: list index out of range
 #17 0x00007f6bb9b389e0 in PyFunction_AsCPointer (pyobject=<optimized out>, pyobject
entry=0x7f6bb8f838c0, rettype="Double_t", signature="(Double_t*,Double_t*)") at /builddir/build/BUILD/root-6.34.02-build/root-6.34.02/bindings/pyroot/cppyy/CPyCppyy/src/Converters.cxx:2713
 #18 0x00007f6bb9b39851 in CPyCppyy::(anonymous namespace)::FunctionPointerConverter::SetArg (this=0x55eb25d61d40, pyobject=0x7f6bb8f838c0, para=..., ctxt=0x7ffc484855c0) at /builddir/build/BUILD/root-6.34.02-build/root-6.34.02/bindings/pyroot/cppyy/CPyCppyy/src/Converters.cxx:2768

(cherry picked from commit a87b474)
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.

4 participants