radical-cybertools / radical.pilot

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

Feature/raptor api 2 #2892

Closed andre-merzky closed 1 year ago

andre-merzky commented 1 year ago

This is a somewhat major overhaul of Raptor and addresses several issues:

One notable semantic change is that the result_cb is called before AGENT_STAGING_OUTPUT - raptor is an executor, and just like the request_cb is called before AGENT_EXECUTING_PENDING, the result_cb is similar bound to the AGENT_EXECUTING scope.

TODO:

This fixes #2731

andre-merzky commented 1 year ago

NOTE: this PR contains #2885 and thus should be merged after that PR.

eirrgang commented 1 year ago

Hmm, that does not seem to raise a deprecation warning just yet:

Oh! You are right. DeprecationWarning is usually suppressed without -X dev or another activating case, but it seems that importing and using the deprecated Generics do not raise a run time DeprecationWarning, is consistent with the expectation that typing hints are as close to no-op at runtime as possible.

From PEP-585:

Importing those from typing is deprecated. Due to PEP 563 and the intention to minimize the runtime impact of typing, this deprecation will not generate DeprecationWarnings. Instead, type checkers may warn about such deprecated usage when the target version of the checked program is signalled to be Python 3.9 or newer. It’s recommended to allow for those warnings to be silenced on a project-wide basis.

The deprecated functionality may eventually be removed from the typing module. Removal will occur no sooner than Python 3.9’s end of life, scheduled for October 2025.

So we've got a few years.

eirrgang commented 1 year ago

At this point in the code we consider it safe to assume that the description targets a worker, so we set it to make sure the downstream code knows what to do. Consider it a service to the end user :-)

Okay... but that means it is not an error to set mode=rp.TASK_EXECUTABLE, and for a Task to have its values silently altered? I don't foresee any real problems, but it seems like a missed opportunity to catch logic errors.

andre-merzky commented 1 year ago

At this point in the code we consider it safe to assume that the description targets a worker, so we set it to make sure the downstream code knows what to do. Consider it a service to the end user :-)

Okay... but that means it is not an error to set _mode=rp.TASKEXECUTABLE, and for a Task to have its values silently altered? I don't foresee any real problems, but it seems like a missed opportunity to catch logic errors.

Yeah, we can issue a warning in those cases.

andre-merzky commented 1 year ago

@mtitov : this is ready for another round of reviews. Thanks!

codecov[bot] commented 1 year ago

Codecov Report

Attention: Patch coverage is 27.42317% with 307 lines in your changes missing coverage. Please review.

Project coverage is 40.66%. Comparing base (f506c52) to head (9b327c9). Report is 3276 commits behind head on devel.

Files with missing lines Patch % Lines
src/radical/pilot/raptor/master.py 25.53% 105 Missing :warning:
src/radical/pilot/raptor/worker.py 5.97% 63 Missing :warning:
src/radical/pilot/task_manager.py 7.81% 59 Missing :warning:
src/radical/pilot/pilot.py 25.00% 12 Missing :warning:
src/radical/pilot/raptor_tasks.py 47.82% 12 Missing :warning:
src/radical/pilot/task_description.py 53.84% 12 Missing :warning:
src/radical/pilot/raptor/worker_default.py 20.00% 8 Missing :warning:
src/radical/pilot/agent/agent_n.py 0.00% 6 Missing :warning:
src/radical/pilot/agent/agent_0.py 0.00% 5 Missing :warning:
src/radical/pilot/agent/executing/base.py 80.76% 5 Missing :warning:
... and 6 more
Additional details and impacted files ```diff @@ Coverage Diff @@ ## devel #2892 +/- ## ========================================== - Coverage 40.95% 40.66% -0.29% ========================================== Files 94 95 +1 Lines 10275 10451 +176 ========================================== + Hits 4208 4250 +42 - Misses 6067 6201 +134 ```

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