search

radical-cybertools / radical.pilot

RADICAL-Pilot
http://radical-cybertools.github.io/radical-pilot/index.html
Other
54 stars 23 forks source link

add documentation for missing properties #2873

Closed andre-merzky closed 1 year ago

andre-merzky commented 1 year ago

This fixes #2734

codecov[bot] commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 40.18%. Comparing base (70dbc48) to head (2b8ebdc). Report is 3349 commits behind head on devel.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## devel #2873 +/- ## ========================================== - Coverage 40.18% 40.18% -0.01% ========================================== Files 94 94 Lines 10262 10260 -2 ========================================== - Hits 4124 4123 -1 + Misses 6138 6137 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

andre-merzky commented 1 year ago

Do we document all the sandboxes we use for the pilot and the task? For example, does a developer know what are the differences/goal among/of resources, session, pilot and client sandboxes? If yes, then I am wondering whether we should add that information in the methods but, seeing the duplication between pilot/task, maybe adding it somewhere else?

One place related to this documentation is the docstring for complete_url. Since this is a free function in the RP module and not part of the public API I am hesitant to link that in too many places - that text might be worthwhile to extract and move elsewhere I guess - but where?

Related but likely not affecting this PR. We documented client/agent sandboxes in the user-facing documentation. Is that enough or does a user need to know about the other sandboxes?

I guess I would reference the same text as above (docstring for complete_url)

mturilli commented 1 year ago

One place related to this documentation is the docstring for complete_url. Since this is a free function in the RP module and not part of the public API I am hesitant to link that in too many places - that text might be worthwhile to extract and move elsewhere I guess - but where?

Happy to consider any suggestion you may have and then see whether I have any useful opinion to contribute. I do not have a sufficient understand of the whole code base to offer useful suggestions :/

Related but likely not affecting this PR. We documented client/agent sandboxes in the user-facing documentation. Is that enough or does a user need to know about the other sandboxes?

I guess I would reference the same text as above (docstring for complete_url)

I am a bit lost. You are suggesting a possible solution to a question that still needs to be answered. Do you think users should know about all the sanboxes? If yes, how that knowledge would help them? The answer to that question would be the base to document them in the user-facing documentation. For example, picking up from another PR, users need to know about the task ranks property because when they need to run MPI tasks on say N nodes, they need to define that property accordingly. Analogously, in what situation users would need to know that there is a session, task, etc. sandbox?

andre-merzky commented 1 year ago

I opened a separate issue (#2887) for this to keep this PR focused for the ScaleMS sprint. Does that work for you @mturilli ?

mturilli commented 1 year ago

@andre-merzky yes, it does, thanks.