New project item resource passing mechanism - [merged] #1164

Closed spine-o-bot closed 3 years ago

In GitLab by @soininen on Nov 8, 2019, 12:51

_Merges issue_418_new_resource_passingmechanism -> dev

This is a proposal implementation for the two phase DAG execution discussed in #418. The goal is to crystallize how project items communicate their available resources and how data is transferred between them.

In this MR, project execution has been split into two phases:

Available project item resources are collected into a new bookkeeping object ResourceMap. The execution simulation infrastracture previously used to advertise the resources has been repurposed for this.
The DAG is executed as before with the exception that each item receives a list of resources available from upstream and downstream items. The resource objects carry a URL with them. Data between the items is passed in files or network resources (e.g. a database) pointed by that URL. This way each item executes in total isolation.

Writing to a database has been moved from Data Store to Importer. This way Data Stores, as well as Data Connections and Views have become 'passive items' in the sense that their execute() method does nothing and they are there just to provide URL resources to other items.

Please note that on contrary to what has been previously stated, resource object can only be used to pass 'static' data between the objects during execution, i.e. only data available during phase 1. can be passed in ProjectItemResource. Execution time data needs to be communicated using files instead. Currently it is unclear if passing data in memory is actually needed. If such a need rises, however, we may want to consider other options.

To test:

This change should not break any previously working DAGs. Testers should try executing their existing workflows and check that:

Execution works as before
Exporters write their output files
Importers import their input files to the database
Tools find their input files, input databases and produce their output files as well as write to their output databases as before

In GitLab by @soininen on Nov 11, 2019, 08:28

added 20 commits

e2014df8...fb4dd1ef - 19 commits from branch dev
746e0099 - Merge branch 'dev' into issue_418_new_resource_passing_mechanism

Compare with previous version

In GitLab by @soininen on Nov 11, 2019, 13:08

added 3 commits

746e0099...403015f3 - 2 commits from branch dev
e0c04779 - Merge branch 'dev' into issue_418_new_resource_passing_mechanism

Compare with previous version

In GitLab by @soininen on Nov 11, 2019, 13:09

changed the description

In GitLab by @manuelma on Nov 11, 2019, 23:05

Awesome. Will test this tomorrow. I was actually looking forward to this with hopes that Importer and Exporter take longer to execute now, so the animations I've been working on have time to play...

In GitLab by @soininen on Nov 12, 2019, 08:19

Having animations would mean that the execution work is done in a separate thread, right? That would be very nice indeed as the execution should not block the Toolbox UI.

In GitLab by @manuelma on Nov 12, 2019, 13:13

We're not passing the resource_map to ExecutionInstance in the executed_selected method.

Traceback (most recent call last):
  File "/home/manuelma/Codes/spine/toolbox/spinetoolbox/widgets/toolbars.py", line 122, in execute_selected
    self._toolbox.project().execute_selected()
  File "/home/manuelma/Codes/spine/toolbox/spinetoolbox/project.py", line 386, in execute_selected
    self.execution_instance = ExecutionInstance(self._toolbox, ordered_nodes)
TypeError: __init__() missing 1 required positional argument: 'resource_map'

In GitLab by @soininen on Nov 12, 2019, 13:26

added 1 commit

2e90ceb1 - Fix execution of selected project items

Compare with previous version

In GitLab by @soininen on Nov 12, 2019, 13:28

Thanks of the heads-up! Should be fixed now although I found an unrelated bug in Exporter which I will fix elsewhere.

In GitLab by @manuelma on Nov 12, 2019, 13:35

Traceback (most recent call last):
  File "/home/manuelma/Codes/spine/toolbox/spinetoolbox/executioner.py", line 379, in item_execution_finished
    self.execute_project_item(item_name)
  File "/home/manuelma/Codes/spine/toolbox/spinetoolbox/executioner.py", line 356, in execute_project_item
    self.running_item.execute()
  File "/home/manuelma/Codes/spine/toolbox/spinetoolbox/project_items/data_store/data_store.py", line 486, in execute
    db_map.commit_session("imported with mapper")
  File "/home/manuelma/Codes/spine/data/spinedb_api/diff_database_mapping_commit_mixin.py", line 37, in commit_session
    raise SpineDBAPIError("Nothing to commit.")
spinedb_api.exception.SpineDBAPIError: Nothing to commit.

To reproduce:

Create a DC, Importer, Data Store, links from DC to Importer, and from Importer to DS.
Add reference to DC.
Open the import editor in the Importer and just check the item in the Sources, then press Save and Close.
Run project.

In my opinion is enough to just catch the exception and log the error message.

In GitLab by @manuelma on Nov 12, 2019, 13:37

Traceback (most recent call last):
  File "/home/manuelma/Codes/spine/toolbox/spinetoolbox/executioner.py", line 380, in item_execution_finished
    item_name = self.execution_list.pop(0)
IndexError: pop from empty list

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/manuelma/Codes/spine/toolbox/spinetoolbox/project.py", line 468, in graph_execution_finished
    self.execution_instance = ExecutionInstance(self._toolbox, execution_list)
TypeError: __init__() missing 1 required positional argument: 'resource_map'

To reproduce:

Add an isolated item to the project (don't connect it to any other), and then press Execute project.

In GitLab by @soininen on Nov 12, 2019, 13:48

Strangely, I cannot reprocude the Traceback here. Are you using the latest spinedb_api?

In GitLab by @manuelma on Nov 12, 2019, 13:52

Ah, you're right. I have some local changes. Let me push them....

In GitLab by @manuelma on Nov 12, 2019, 13:54

Should be done now...

Edit: My bad, the changes were already pushed. That wasn't the problem.

The traceback actually appears every time you try to import an 'empty mapping'.

In GitLab by @soininen on Nov 12, 2019, 14:02

added 1 commit

5107ca8e - Fix execution of multiple DAGs

Compare with previous version

In GitLab by @soininen on Nov 12, 2019, 14:03

Again I had forgotten to create and pass the resource map around. Should be fixed now.

In GitLab by @soininen on Nov 12, 2019, 14:07

Now I can see the Traceback. I think my Data Store just didn't have a database previously.

In GitLab by @manuelma on Nov 12, 2019, 14:48

Do we need more testers? I wasn't able to test the Exporter fully because I don't have GAMS, but I think it's ok to merge.

In GitLab by @soininen on Nov 12, 2019, 14:56

I'll try to fix the Traceback issue before merging. Perhaps I should try to add some unit tests for the execution methods in SpineToolboxProject as well to make sure they don't break again.

In GitLab by @manuelma on Nov 12, 2019, 15:00

Yes, sorry, I meant after fixing the traceback issue, it's ok to merge. At least I couldn't find anything else.

In GitLab by @soininen on Nov 12, 2019, 15:01

I get some strange lines in the Logs when loading project/adding new items:

[BUG] Could not find a graph containing Data Connection 1. Please reopen the project.

I'll investigate this as well.

In GitLab by @soininen on Nov 12, 2019, 15:49

added 3 commits

e26951f2 - Catch SpineDBAPIErrors when commiting. Re #418
d4d2e5af - Add unit tests for SpineToolboxProject execution.
a1af2979 - Fix superfluous error logging while adding items

Compare with previous version

In GitLab by @soininen on Nov 12, 2019, 15:50

This was a separate issue but I fixed it here nonetheless.

In GitLab by @soininen on Nov 12, 2019, 15:51

All know issues this far have been fixed.

In GitLab by @manuelma on Nov 12, 2019, 15:55

I feel like pressing the button...

In GitLab by @soininen on Nov 12, 2019, 16:02

I was actually hoping for some comments on how well this would work with the Spine Engine in mind but maybe it is best to merge and see how things develop. At least this MR cleans the resource discovery/execution part a bit in my opinion, so that is an improvement already.

In GitLab by @manuelma on Nov 12, 2019, 16:17

To me it's definitely an improvement and actually the proof-of-concept we were looking for. That said, of course I don't mind taking it slowly and collect more comments before we merge. But I'd still just merge.

Edit: @PekkaSavolainen @ererkka @fabianoP any comments?

In GitLab by @soininen on Nov 13, 2019, 08:30

added 2 commits

ce43c0e4 - Add ExecutionState.WAIT state
4c9697d2 - Improve SpineToolboxProject unit tests

Compare with previous version

In GitLab by @soininen on Nov 13, 2019, 08:36

I made some small changes to make the special Tool execution officially supported by the execution infrastructure.

In GitLab by @manuelma on Nov 13, 2019, 08:46

What is the special tool execution?

In GitLab by @soininen on Nov 13, 2019, 08:52

The actual execution is done in a separate process and the execution instance has to wait for that process to finish. Previously, Tool.execute() did not emit ExecutionInstance.project_item_execution_finished_signal. The signal was emitted in Tool.handle_execution_finished which then called project_item_execution_finished_signal.emit(ExecutionState.CONTINUE).

Now, Tool.execute() emits ExecutionState.WAIT which tells the execution instance to wait for another signal from the item. Currently, we don't do much with the information although it makes Tool more consistent with the other items. In the future, other items may offload their execution to separate processes in a similar way so I though we should prepare for that.

In GitLab by @manuelma on Nov 13, 2019, 09:35

Excellent, I like it a lot.

In GitLab by @manuelma on Nov 13, 2019, 12:42

Consider the DAG below and corresponding Event log:

Screenshot_from_2019-11-13_11-41-51

[13-11-2019 11:39:38] --------------------------------------------------
[13-11-2019 11:39:38] Executing Selected Directed Acyclic Graph
[13-11-2019 11:39:38] Order: Skellefte -> Spine Model -> View 1
[13-11-2019 11:39:38] --------------------------------------------------
[13-11-2019 11:39:38] 
[13-11-2019 11:39:38] Executing Data Store Skellefte
[13-11-2019 11:39:38] ***
[13-11-2019 11:39:38] 
[13-11-2019 11:39:38] Executing Tool Spine Model
[13-11-2019 11:39:38] ***
[13-11-2019 11:39:38] *** Copying Tool specification SpineModel source files to work directory ***
[13-11-2019 11:39:38]   Copied 1 file(s)
[13-11-2019 11:39:38] *** Executing in work directory mode ***
[13-11-2019 11:39:38] *** Checking Tool specification requirements ***
[13-11-2019 11:39:38] *** Searching for required input files ***
[13-11-2019 11:39:38] *** Copying input files to work directory ***
[13-11-2019 11:39:38]   Copying file input.sqlite
[13-11-2019 11:39:38]   Copied 1 input file(s)
[13-11-2019 11:39:38] *** Starting instance of Tool specification SpineModel ***
[13-11-2019 11:39:38]   Initializing Julia...
[13-11-2019 11:39:38]   Julia version is 1.1.0
[13-11-2019 11:39:38] *** Starting Julia Console ***
[13-11-2019 11:39:38] 
[13-11-2019 11:39:38] Executing View View 1
[13-11-2019 11:39:38] ***
[13-11-2019 11:39:38] DAG 1/1 finished
[13-11-2019 11:39:38] Execution complete
[13-11-2019 11:39:44]   Julia REPL successfully started using kernel specification julia-1.1
[13-11-2019 11:39:44]   Execution in progress. See Julia Console for messages.

I'm seeing the message Execution complete before execution is actually started. I haven't tried it in dev, thought. I don't know if it's particular to this branch.

In GitLab by @soininen on Nov 13, 2019, 12:47

Indeed, I seem to have missed this. It works in dev so I guess I've broken something here. I'll investigate. Thanks for pointing it out!

In GitLab by @soininen on Nov 13, 2019, 13:02

added 1 commit

9328cbaf - Use correct parameter types for signals

Compare with previous version

In GitLab by @soininen on Nov 13, 2019, 13:03

Should be fixed now, could you please confirm? I think the issue was actually present in dev but it was just hidden and the introduction of ExecutionState.WAIT made it appear. I've still got a lot to learn about Qt, it seems.

In GitLab by @manuelma on Nov 13, 2019, 13:08

It's gone, good job.

[13-11-2019 12:05:47] --------------------------------------------------
[13-11-2019 12:05:47] Executing Selected Directed Acyclic Graph
[13-11-2019 12:05:47] Order: Skellefte -> Spine Model -> View 1
[13-11-2019 12:05:47] --------------------------------------------------
[13-11-2019 12:05:47] 
[13-11-2019 12:05:47] Executing Data Store Skellefte
[13-11-2019 12:05:47] ***
[13-11-2019 12:05:47] 
[13-11-2019 12:05:47] Executing Tool Spine Model
[13-11-2019 12:05:47] ***
[13-11-2019 12:05:47] *** Copying Tool specification SpineModel source files to work directory ***
[13-11-2019 12:05:47]   Copied 1 file(s)
[13-11-2019 12:05:47] *** Executing in work directory mode ***
[13-11-2019 12:05:47] *** Checking Tool specification requirements ***
[13-11-2019 12:05:47] *** Searching for required input files ***
[13-11-2019 12:05:47] *** Copying input files to work directory ***
[13-11-2019 12:05:47]   Copying file input.sqlite
[13-11-2019 12:05:47]   Copied 1 input file(s)
[13-11-2019 12:05:47] *** Starting instance of Tool specification SpineModel ***
[13-11-2019 12:05:47]   Initializing Julia...
[13-11-2019 12:05:48]   Julia version is 1.1.0
[13-11-2019 12:05:48] *** Starting Julia Console ***
[13-11-2019 12:05:53]   Julia REPL successfully started using kernel specification julia-1.1
[13-11-2019 12:05:53]   Execution in progress. See Julia Console for messages.
[13-11-2019 12:07:43]   Tool specification execution finished
[13-11-2019 12:07:43] Tool Spine Model execution finished
[13-11-2019 12:07:43] *** Archiving output files to results directory ***
[13-11-2019 12:07:43]   The following output files were saved to results directory
[13-11-2019 12:07:43]       output.sqlite
[13-11-2019 12:07:43] 
[13-11-2019 12:07:43] Executing View View 1
[13-11-2019 12:07:43] ***
[13-11-2019 12:07:43] DAG 1/1 finished
[13-11-2019 12:07:43] Execution complete

In GitLab by @manuelma on Nov 13, 2019, 13:14

Minor things like the above might still appear but are not dramatic and it seems @soininen can fix them real fast.

At the same time, we don't need to have it in a separate branch to hear comments about how it blends in with the ideas for the engine. We can merge, let everybody try it in dev, and discuss possible implications in the next engine meeting.

What do you say @soininen? Should I just press it?

In GitLab by @soininen on Nov 13, 2019, 13:15

Agreed.

In GitLab by @manuelma on Nov 13, 2019, 13:16

merged

In GitLab by @manuelma on Nov 13, 2019, 13:16

mentioned in commit 2f9634efad746ff4e168118cce313b2b515ab302

spine-tools / Spine-Toolbox

New project item resource passing mechanism - [merged] #1164