[WIP] adding tasks/assignments dynamically

[salelkafrawy]

I've started with Jack's first suggestion in the issue#99:

• create a main thread that is responsible of running both: try_launch_units() and try_create_assignments() threads.
• I doubt that generate_assignment() with its current version will work if self.assignment_data_list is changing its size (which is what I expect as the required task is to allow dynamic size of this list) but I need to think about how to test it to know if it works.

[JackUrb]

A bit of a miscommunication on design here - the thought is that if either of the parameters that would require a generator thread be set up, the TaskLauncher would run a single thread to keep track of the generators. Otherwise, it'd run regularly (without any threads).

Operational flags to keep track of would be what type of generation would be needed (perhaps an enum of none, unit, or assignment (as if the assignment is a generator, the units must be as well since more units will be added later)), and what parts have been launched (did_launch_assignments, and did_launch_units for instance).

If it's running as a generator, you would set up a single thread to act as the main at the end of __init__. This thread would check the values of did_create_assignments and did_launch_units respectively in order to decide whether or not to call _try_launch_units and _try_create_assignments.

TaskLauncher itself is not turning assignment_data_list into a generator. You can use isinstance(assignment_data, types.GeneratorType) (after importing types) to see if the provided list is a generator.

The only way to test this initially will be by writing a new test case. It will define a generator (like the generate_units you've created here) and pass that instead of a list as the argument for assignment_data_list (which we should probably rename to assignment_data_iterator).

[JackUrb]

Generally looking closer, though as we discussed a bit back it may make sense to just have two generator threads.

[JackUrb]

This is beginning to look correct from an implementation perspective - what do the tests say?

[JackUrb]

Alright this is 99% of the way there - the only remaining thing to do is ensure that you join the generator threads at the end of expire_units. You'll be able to tell you've done this properly when test/core/test_operator.py passes.

[salelkafrawy]

@JackUrb I added the thread.join() for both threads at the end of expire_units() but test/core/test_operator.py still fails (there are 3 threads not 1 as expected), that's because expire_units() in TaskLauncher is never called from operator.py; at this line:

Mephisto/mephisto/core/operator.py

Line 307 in 10a9d10

for tracked_run in self._task_runs_tracked.values():

the number of items in self._task_runs_tracked dictionary is zero.

[JackUrb]

Ah this is a fair point - the correct course of action here would be to move the flag-setting and joining to a new shutdown() function, and call that from both operator.shutdown() (in the line before the tracked_run.task_launcher.expire_units() call) and within operator._track_and_kill_runs() after the tracked_run.architect.shutdown() call.

Having a shutdown tends to be a better design for cleaning up allocated resources anyways

[JackUrb]

Why change these times?

[salelkafrawy]

there was a test in the test_operator.py making sure that consecutive assignments finish after certain amount of time, when they are as large as before it fails, so I changed it and was about to ask you (how can we define these numbers efficiently? and should the test in the test_operator.py be changed later?)

[JackUrb]

I'd update the test_operator to use a timeout style test rather than a direct check, like the style of:

start_time = time.time()
while (condition is not met):
  assert time.time - start_time is not more than some amount

as the timeouts defined initially here seem better than these really rapid ones.

[JackUrb]

So do the operator tests pass now? Have you made that change as well?

[salelkafrawy]

Now test_operator.py passes .. I just had to increase the TIMEOUT_TIME to match the UNIT_GENERATOR_WAIT_SECONDS.

[JackUrb]

Looks ready to go at this point! Thanks for bearing with me through all the changes.

[salelkafrawy]

@JackUrb Thank you for bearing with me and mentoring me and teaching me alot :)