tecan / ttc21incrementalLabWorkflows

This repository contains the code for the Incremental Laboratory Workflow case at the TTC 2021
MIT License
0 stars 3 forks source link

Review of YAMTL solution #7

Open georghinkel opened 3 years ago

georghinkel commented 3 years ago

First, sorry for being late with this review. I was on a parental leave and unexpectedly did not have an internet connection.

I was not able to run the solution, yet. Will do so hopefully during today, though I actually expect everything to work on that end.

The solution is generally easily understandable and very performant. I especially like the idea of the toMany rules with the ability to trace the correct element from other rules.

The only thing, I do not understand the transformation of the next/prev pointers. To me, it looks like the transformation is creating next/prev links for all transformed low-level jobs, not just the ones that operate on the same samples.

arturboronat commented 3 years ago

Congratulations for the newborn! And thanks for taking the time to write comments.

Rule job shows how to set previous jobs in the low level model:

rule('job').isAbstract.toMany
    .in('in_step', LAB.protocolStep)
    .out('out_job', JOB.job) [
        out_job.protocolStepName = in_step.id
        // set container    
        val in_jobRequest = in_step.eContainer.eContainer as JobRequest
        val out_jobCollection = in_jobRequest.fetch('out_jobCollection', 'jobRequest_->_jobCollection') as JobCollection
        out_jobCollection.jobs.add(out_job)

        if (in_step.previous !== null)
            out_job.previous.add(in_step.previous.fetch() as Job)   
    ],

Specifically, in_step.previous.fetch() will fetch the job that corresponds to the previous protocol step so that jobs preserve the order specified among steps. I assumed that the jobs that were created for the same protocol step can be performed concurrently and these may share the same previous/next jobs.

Listing 7 in the paper refers to old code. During the experimentation with rules toMany the fetch operator could return a list of objects (corresponding to output objects of all matches) by default but returning the output object of the first match, when the match occurrence is not specified, seems more intuitive.

fjouault commented 3 years ago

This declarative YAMTL solution is written in an embedded xtend DSL and achieves a relatively good scalability.

The fact that xtend is imperative slightly increases verbosity compared to what could be achieved with an external DSL. However, an advantage is that xtend is directly usable.

When running the solution, we observed that it crashed with an exception on some models from the new_samples set. We also noticed that it did not save target models, but we managed to modify the code to make it do so.

We noticed the following issues in the result models. These were not detected by the NMF-based validation program.

Minor comments on the paper:

This review was written in collaboration with @TheoLeCalvar.

arturboronat commented 3 years ago

Many thanks @fjouault and @TheoLeCalvar, the solution is crashing when new samples are added indeed. I need to look into these issues.

georghinkel commented 3 years ago

@arturboronat Actually, the previous/next references are a bit more sophisticated: A low-level job is a next job of another if the original high-level protocol step is next of the other and both jobs share at least one sample they are operating on.

That is, if a low-level job was responsible to distribute samples 1 and 2, then the add reagent to samples 3 and 4 is not a next job, even if the add reagent step was the next after the distribute. In reality, this sharpness is critical because time constraints cannot be met otherwise.

arturboronat commented 3 years ago

Thanks all for your comments.

I have uploaded a fix to the solution where the main modifications are:

Regarding major issues:

I have also included a docker image.

The commit is here.

See you all tomorrow.