[root6] Add include statements to StarDb macros #439
Conversation
Before submitting PR#315 some code cleanup was performed (see note below). The fast jet directory was moved from StarGenerator/FastJetFilt to StarGenerator/FILT/FastJetFilt, to conform with the standard source tree layout for filters. 4f27034 625bbdd Conscript-standard directory rules allow one to specify include paths based on the module (StarGenerator) and package. The package is one directory beneath the module... before code cleanup this was the FastJetFilter directory. After cleanup, we have to apply the fast jet include path to all filters in the StarGenerator/FILT directory. ---- Note on the pp 200 HF filtered jet production. The production commenced with fast jet filter code built before code cleanup, based on the original location of the fast jet filter in the source tree.
when we use the table classes in these macros. Include statemets are wrapped in an ifdef __CLING__ statement -- includes are problematic for ROOT5/cint... possibly because of the default search paths that are configured, but... -- ROOT5/cint can live without the includes b/c it is interpreted and has the class info via the dictionaries
klendathu2k
requested review from
kkauder,
rkunnawa,
Navagyan,
iraklic,
genevb,
Morning-Ye,
fgeurts,
dmarkh and
perevbnlgov
as code owners
Since you mention it, is it something that is not covered by the CI tests? |
[skip ci] Co-authored-by: Dmitry Kalinkin <[email protected]>
The root6 ci on github ignores the failures. It was setup to track the progress for porting. I can't see how this PR alone can improve the current crashes. |
I am very well aware of that. That's why I want Jason to clarify what he means by that statement. (I didn't even know that nightly tests had IDs one could use to refer to them) |
|
This PR is necessary, but not sufficient, to make the nightly tests / CI tests run. It has been tested under root 5 to confirm that our code still works. For root 6, there are a couple of other changes that need to be made to the code to get to the point where this PR is necessary. These will be future PR's. |
I think ROOT5 cannot find the includes because the search paths are not set properly for the interpreter. Hopefully, #441 can solve this, and if it works for ROOT5 we can leave out the |
No objections if it works. |
Yes, I confirm that the missing types supposed to be generated for those DB tables are not used anywhere in StRoot/ |
|
Search paths are causing a side effect that is doesn't let me run the nightly tests... but I can avoid loading the rootlogon and keep testing. I think we can merge. |
It is something for the Production team to figure out preferably before we merge. I don't know how they set up .rootrc or rootlogon.C/rootlogoff.C macros on the farm. In the CI jobs we obviously use these the current revisions from StRoot/macros/ |
|
The production environment is just like any other STAR user environment on the SDCC farm. If a different approach is being used for github CI tests, I don't see figuring that out as a Production Team responsibility.
Thanks,
-Gene
… On Nov 16, 2022, at 10:08 AM, Dmitri Smirnov ***@***.***> wrote:
Search paths are causing a side effect that is doesn't let me run the nightly tests... but I can avoid loading the rootlogon and keep testing.
It is something for the Production team to figure out preferably before we merge. I don't know how they set up .rootrc or rootlogon.C/rootlogoff.C macros on the farm. In the CI jobs we obviously use these the current revisions from StRoot/macros/
|
Of course not. You can't possibly know how users set up their environments.
So, what is the right approach? Could you elaborate and guide us through if you think this is some else's responsibility. |
|
Hi, Dmitri
On Nov 16, 2022, at 12:11 PM, Dmitri Smirnov ***@***.***> wrote:
The production environment is just like any other STAR user environment on the SDCC farm.
Of course not. You can't possibly know how users set up their environments.
Indeed, I left out the word "default" before "STAR user environment". Thanks for the correction as we do not prevent people from customizing their own.
If a different approach is being used for github CI tests, I don't see figuring that out as a Production Team responsibility.
So, what is the right approach? Could you elaborate and guide us through if you think this is some else's responsibility.
Two items I can think of:
(1) The Production Team does not manage/maintain the default STAR user environment. That has been a task under the care of Jerome historically as an infrastructure task, not as a production task. The Production Team cares if the environment changes in an impactful way and may have to make adjustments for such changes as one of the users, albeit a very important user.
(2) The default STAR user environment on SDCC is, to my knowledge, considered the official environment under which STAR software must work. If the environment on github for CI differs from that, I expect the burden is on the maintainer of the non-official environment to match the official environment. But if anyone identifies something about the official environment that could benefit from a specific improvement, see item (1).
Thanks,
-Gene
|
|
Gene, you are leaving out a very important point. It is the production team who installs the STAR software on the farm. As far as I know nobody else can do it. So, for this change to work properly and to move us closer to ROOT6, the production team (as one of the users of the STAR software) needs to either use the StRoot/macros/rootlogon.C provided in the code tree or update their own accordingly. ROOT also provides a mechanism to pick a specific rootlogon/rootlogoff.C via .rootrc. Again, you can use the one in StRoot/macros or come up with your own or suggest changes to all of these files. These are the handles to run the code but if you have a better suggestion how to accommodate this change please let us know. |
|
The production user starreco does not use a custom environment:
root -l -b
*** Float Point Exception is OFF ***
*** Start at Date : Wed Nov 16 15:30:55 2022
QAInfo:You are using STAR_LEVEL : pro, ROOT_LEVEL : 5.34.38 and node : rcas6003.rcf.bnl.gov
root.exe [0] .which .rootrc
/afs/rhic.bnl.gov/star/packages/SL22b/StRoot/macros/.rootrc
root.exe [1] .which rootlogon.C
/afs/rhic.bnl.gov/star/packages/SL22b/StRoot/macros/rootlogon.C
Does that help?
-Gene
… On Nov 16, 2022, at 3:12 PM, Dmitri Smirnov ***@***.***> wrote:
the production team (as one of the users of the STAR software) needs to either use the StRoot/macros/rootlogon.C provided in the code tree or update their own accordingly. ROOT also provides a mechanism to pick a specific rootlogon/rootlogoff.C via .rootrc. Again, you can use the one in StRoot/macros or come up with your own or suggest changes to all of these files.
|
I don't know. If you say it is safe to merge this we'll do it. I thought something did not work for Jason... |
|
On Nov 16, 2022, at 3:53 PM, Dmitri Smirnov ***@***.***> wrote:
Does that help?
I don't know. If you say it is safe to merge this we'll do it.
I thought something did not work for Jason...
I do not know whether it is safe to merge in a final sense. I do not know the details of Jason's tests, like whether his search paths are what they would be in the default STAR dev environment. It is certainly preferable that Jason or another user attempts to replicate that environment for testing, but a fall-back option may be to allow a quickly-reversible merge to go through to DEV for one night (we are not running FastOffline, so DEV offers a little more flexibility now than during experiment Runs).
…-Gene
|
|
If Jerome had left STAR, why we still can't just give someone else the permissions and assign the responsibility? |
|
Sorry, I lost track of some of this discussion. When the include paths are added via the root logon... root 6 complains when trying to autoload headers. So there is a conflict somewhere that i don't understand. And I infer from other parts of this discussion that it is environment dependent. So ... I would propose that the rootlogon change be a separate pull request, which we address after setting up an intial test library. |
#441 ? It is already merged I don't understand what problem you are experiencing. If you need help please provide a how to reproduce example. |
I guess the issue was unrelated to the include paths. Or my environment doesn't load in the rootlogon. I am assuming that this PR #439 is still to be merged? Or is the belief that #441 will resolve the problem with StarDb? |
|
#441 is needed to get rid of However, Jason your own comments above indicated that there might be a problem with using this on the farm. I warned the production team that they may have a similar problem. That is it. If the production team does not want to test or double check this before it goes into main, it is their choice. As I noted above, I don't even understand what the problem is or if there is a problem :) |
|
Well... the CLING guard was in place just to isolate this change to root 6... Let's move forward on everything else but this PR. I can then test to see if the issue I was seeing when I tried the new root logon was real or pilot error. And then move forward from there. |
Yes, and as you can see from the CI tests it works perfectly for ROOT5 too. We don't want to create branches in our code unnecessarily |
plexoos
changed the title
Fix error: unknown type name 'St_DataSet' in ROOT6 tests
```
LoadTable: .L /star-sw/StarDb/tpc/tsspars/tsspar.y2019.C
In file included from input_line_851:1:
/star-sw/StarDb/tpc/tsspars/tsspar.y2019.C:3:1: error: unknown type name 'St_DataSet'
St_DataSet *CreateTable() {
^
/star-sw/StarDb/tpc/tsspars/tsspar.y2019.C:57:10: error: use of undeclared identifier 'St_DataSet'
return (St_DataSet *)tableSet;
^
/star-sw/StarDb/tpc/tsspars/tsspar.y2019.C:57:22: error: expected expression
return (St_DataSet *)tableSet;
^
root4star: .sl79_gcc485/OBJ/StRoot/St_db_Maker/St_db_Maker.cxx:932: virtual TDataSet* St_db_Maker::LoadTable(TDataSet*): Assertion `!ee' failed.
sh: line 1: 133 Aborted (core dumped) MALLOC_CHECK_=3 root4star -b -q -l 'bfc.C(50, "pp2022,StiCA,BEmcChkStat,ftt,-picowrite,-hitfilt,-evout", "/star/rcf/test/daq/2022/040/23040001/st_fwd_23040001_raw_0000003.daq")'
Error: Process completed with exit code 134.
```
Similar to star-bnl#439 we add includes to newly added macros
ROOT6/cling needs to have include files for the table classes used in these macros.
This PR consists of one single change... to many files: Adding the lines
at the top of each macro. We only make this change for ROOT6/cling, as ROOT5/cint is
happy with the status-quo. (And ROOT5 does not like the added includes.)
This change has been tested for a single nightly test job (job 10).