radical-cybertools / radical.saga

A Light-Weight Access Layer for Distributed Computing Infrastructure and Reference Implementation of the SAGA Python Language Bindings.
http://radical-cybertools.github.io/saga-python/
Other
83 stars 34 forks source link

fix id based __eq__ #809

Closed andre-merzky closed 4 years ago

mtitov commented 4 years ago

agree with these made changes and have more to confirm: currently we have t.get_id() == id in Container.get_task here, and originally it was t.id (before I updated it to use _id which was taken from Task.get_id). Thus would t.get_id give t.id (which will work only for Job instances)? or should we make it explicit:

        for t in self.tasks:
            task_id = getattr(t, 'id', t._id)
            if task_id == id:
                return t
andre-merzky commented 4 years ago

Yeah, I kind of expected that you stumble over that line :-) I am a bit hesitant to change more code, and we need to get this out fast. But then again it looks good... Please do some small tests and commit if you are confident that the multi-pilot case remains functional. Small test case:

#!/usr/bin/env python3

import time
import radical.pilot as rp

try:
    session = rp.Session()

    pmgr  = rp.PilotManager(session=session)
    pdesc = rp.ComputePilotDescription({
        'resource': 'local.localhost',
        'runtime': 60,
        'cores': 1
    })

    pilots = pmgr.submit_pilots([pdesc] * 4)

    while True:

        time.sleep(1)

        states = [p.state for p in pilots]
        print(states)

        if rp.PMGR_ACTIVE in states:

            time.sleep(60)

            states = [p.state for p in pilots]
            print(states)
            assert(len(set(states)) == 1), states
            break

except:
    session.close(download=False)
    raise

Thanks!

mtitov commented 4 years ago

@andre-merzky sorry, took some time for me to figure out things! your test works fine, and I see that for Job instances attribute id contains data from get_id. Thus just to keep method get_id inside Container.get_task is enough (works for Task-instances and Task-based such as Job-instances). All good! Thanks!