search

radical-cybertools / radical.analytics

Analytics for RADICAL-Cybertools
Other
1 stars 1 forks source link

radical.pilot is a dependency to be included #110

Closed lee212 closed 4 years ago

lee212 commented 4 years ago

radical.pilot is added to the required packages according to this:

$ bin/radical-analytics-inspect re.session.two.hrlee.018282.0003
re.session.two.hrlee.018282.0003 Traceback (most recent call last):
  File "/home/hrlee/git/radical.analytics/bin/rp_inspect/plot_state.py", line 10, in <module>
    import radical.pilot     as rp
ModuleNotFoundError: No module named 'radical.pilot'
andre-merzky commented 4 years ago

Hey @lee212 - thanks for the PR. We previously intended to keep RP dependence out of RA at installation time, since RA can also analyze non-RP sessions, so it doesn't need to be deployed. But in fact we mostly use it for RP really. @mturilli : what's you opinion? Adding the dep does make installation easier...

andre-merzky commented 4 years ago

PS.: along the same lines we should then also add RE I guess - but in total (RP + RE) that pulls quite a number of dependencies...

mturilli commented 4 years ago

This is a tricky one. In theory, could we use RA only with EnTK if we do not care to profile RP? From a design point of view, this should be possible and I would probably keep it in this way.

andre-merzky commented 4 years ago

This is a tricky one. In theory, could we use RA only with EnTK if we do not care to profile RP? From a design point of view, this should be possible and I would probably keep it in this way.

yes and no: RA can look at RE events only (i.e. ignore RP events), but it needs RE installed which will pull RP as dependency.

What I meant though is that RA can look at RU profiles, and does not need RP or RE for that, so I am hesitant to make it a hard dependency. But, OTOH, it's not our usual use case...

mturilli commented 4 years ago

I would not make it an hard dependency then. The rational is also that RE at the moment requires RP but if it will develop further it may relax this dependency to be able to use different runtime systems. @lee212 would this be a big problem for you?

andre-merzky commented 4 years ago

Makes sense IMHO. @lee212 : if you don't mind, let me rework your PR to at least provide a better message than just an import error...

andre-merzky commented 4 years ago

...rework your PR to at least provide a better message than just an import error...

I did that now. Let's turn the table and let me ask you to review this PR from that perspective? :-)

lee212 commented 4 years ago

Thank you both for the comments and the changes. I reviewed new soft dependency and user-friendly messages. It looks good to me.