Add default processing #3116

antgonza · 2021-06-24T00:58:49Z

First merge #3110; then I'll update this PR.

This PR adds a single button to generate/populate based on our default pipelines, for example:

…-processing

…ssing

coveralls · 2021-06-24T12:36:35Z

Coverage decreased (-0.03%) to 91.089% when pulling 4a97f92 on antgonza:add-default-processing into e30107c on qiita-spots:dev.

wasade · 2021-06-29T16:49:23Z

qiita_db/metadata_template/prep_template.py

+
+    def add_default_workflow(self, user):
+        """The modification timestamp of the prep information
+


Suggested change

wasade · 2021-06-29T16:50:09Z

qiita_db/metadata_template/prep_template.py

+
+        Parameters
+        ----------
+        user : The user that requested to add the default workflows


Would it be possible to note the expected type of user?

wasade · 2021-06-29T16:51:03Z

qiita_db/metadata_template/prep_template.py

+        # 3. loop over the missing merging schemes and create the commands
+        #    missing to get to those processed samples and add them to a new
+        #    workflow
+        # 4.


Suggested change

# 4.

wasade · 2021-06-29T16:55:31Z

qiita_db/metadata_template/prep_template.py

+        """
+        # helper functions to avoid duplication of code
+
+        def _get_node_info(node):


why pass node but not wk?

I could add but when this is called, wk is "global" within this method; do you think is better to pass?

it may be nice to either pass both or pass neither as node is also non-local from what I can tell from the function call

wasade · 2021-06-29T16:57:10Z

qiita_db/metadata_template/prep_template.py

+            if not missing_artifacts[wk]:
+                del missing_artifacts[wk]
+        if not missing_artifacts:
+            raise ValueError('This preparation is complete')


This doesn't seem to correspond to the list of reasons for a ValueError in the docstring, or I may be misunderstanding how this relates to the described reasons

You are right, I'll add it.

wasade · 2021-06-29T16:57:48Z

qiita_db/metadata_template/prep_template.py

+
+        def _get_predecessors(node):
+            # recursive method to get predecessors of a given node
+            for pnode in wk.graph.predecessors(node):


same question here regarding node and wk

Yup, same, they exists in add_default_workflow and the functions are defined and called within that scope so they are "global" ... in fact, we might not even need to pass node. Anyway, I'm OK changing to whatever you think is best, just let me know.

passing both may be nice to be explicit, tracing non-local variables can get confusing quickly

wasade · 2021-06-29T17:01:30Z

qiita_db/metadata_template/prep_template.py

+                    pred = [data]
+                else:
+                    pred.append(data)
+                return pred


I think that having the return in scope of the for would imply a node can only have a single predecessor. Is that correct for both interpretation and intent?

Yes!! Note that this is a recursive function (line 767 calls itself) as we need the "grandparent" information to create the "parent" information.

Ya, but just so I understand, is it correct that a node only ever has a single parent?

Thank you for double checking this; better to be safe.

Yes, I spent sometime looking at this and this is the expected behavior; basically, by being recursive it will go down to the origin of the workflow (always a single artifact) and return (if pred is None:) the single only parent, then it will start coming out of the recursion and appending in order the children jobs (else:). In case it helps, we tested this in qiita-rc and everything seem fine.

wasade · 2021-06-29T17:06:18Z

qiita_db/metadata_template/test/test_prep_template.py

        self.assertEqual(pt.artifact, None)
        artifact = qdb.artifact.Artifact.create(
            self.filepaths, "FASTQ", prep_template=pt)
        self.assertEqual(pt.artifact, artifact)
+
+        # here we can test that we can properly create a workflow


This seems like a limited test given the complexity of the function, are there edge cases beyond the ValueError check below that should be examined?

Good point, I'll add more tests.

Just FYI, I checked everything that came to mind and I think those "simple" tests actually cover all scenarios by having to create a full pipeline and then checking that full pipeline and not having anything else to create.

antgonza

@wasade, thank you for detailed review, I have some questions; then, I'll issue fixes to your comments.

antgonza · 2021-06-29T17:15:58Z

qiita_db/metadata_template/prep_template.py

+        """
+        # helper functions to avoid duplication of code
+
+        def _get_node_info(node):


I could add but when this is called, wk is "global" within this method; do you think is better to pass?

antgonza · 2021-06-29T17:17:21Z

qiita_db/metadata_template/prep_template.py

+
+        def _get_predecessors(node):
+            # recursive method to get predecessors of a given node
+            for pnode in wk.graph.predecessors(node):


Yup, same, they exists in add_default_workflow and the functions are defined and called within that scope so they are "global" ... in fact, we might not even need to pass node. Anyway, I'm OK changing to whatever you think is best, just let me know.

antgonza · 2021-06-29T17:19:09Z

qiita_db/metadata_template/prep_template.py

+                    pred = [data]
+                else:
+                    pred.append(data)
+                return pred


Yes!! Note that this is a recursive function (line 767 calls itself) as we need the "grandparent" information to create the "parent" information.

antgonza · 2021-06-29T17:19:30Z

qiita_db/metadata_template/prep_template.py

+            if not missing_artifacts[wk]:
+                del missing_artifacts[wk]
+        if not missing_artifacts:
+            raise ValueError('This preparation is complete')


You are right, I'll add it.

antgonza · 2021-06-29T17:19:57Z

qiita_db/metadata_template/test/test_prep_template.py

        self.assertEqual(pt.artifact, None)
        artifact = qdb.artifact.Artifact.create(
            self.filepaths, "FASTQ", prep_template=pt)
        self.assertEqual(pt.artifact, artifact)
+
+        # here we can test that we can properly create a workflow


Good point, I'll add more tests.

…ssing

mestaki · 2021-06-30T02:15:59Z

Wasn't sure if this should go here or have its own separate issue, let me know and happy create a new issue.

As per the recent email thread on this topic: the Standard Workflow option is great, but without a brief description of what it actually is/does it might be confusing for users. Adding a brief description of this would be thus very useful. Happy to contribute to this.
Option 1: Add a short description on top of the main network processing page,
Option 2: A brief "hover-over" text box appears over the "Standard Workflow" button,
Option 3: A detailed description in the main help files and just provide a link to that in the network processing page?
Option 4: something else?

Depending on the options the description I can make the text brief or more detailed.

antgonza · 2021-06-30T02:49:21Z

Thank you @mestaki; I think 1 and 3 would be great. If you send me an email with that I can open a PR adding those texts so you can review there ... or you can open a PR with those changes and I can review; whatever works best. BTW no need to open a new issue.

antgonza

Thank you @wasade.

antgonza · 2021-06-30T14:16:24Z

qiita_db/metadata_template/test/test_prep_template.py

        self.assertEqual(pt.artifact, None)
        artifact = qdb.artifact.Artifact.create(
            self.filepaths, "FASTQ", prep_template=pt)
        self.assertEqual(pt.artifact, artifact)
+
+        # here we can test that we can properly create a workflow


Just FYI, I checked everything that came to mind and I think those "simple" tests actually cover all scenarios by having to create a full pipeline and then checking that full pipeline and not having anything else to create.

antgonza · 2021-06-30T14:20:46Z

qiita_db/metadata_template/prep_template.py

+                    pred = [data]
+                else:
+                    pred.append(data)
+                return pred


Thank you for double checking this; better to be safe.

Yes, I spent sometime looking at this and this is the expected behavior; basically, by being recursive it will go down to the origin of the workflow (always a single artifact) and return (if pred is None:) the single only parent, then it will start coming out of the recursion and appending in order the children jobs (else:). In case it helps, we tested this in qiita-rc and everything seem fine.

wasade · 2021-06-30T14:56:46Z

Good to merge?

antgonza · 2021-06-30T14:57:51Z

Yes, thank you. The tutorial/text updates will be done in a separate PR (together with the release notes).

antgonza added 4 commits June 23, 2021 12:09

add-default-processing

ccedfc8

Merge branch 'fix-3100' of github.com:antgonza/qiita into add-default…

6510d56

…-processing

minor fixes

c3c5e1c

add tests

910f1e8

antgonza requested a review from wasade June 24, 2021 00:58

antgonza added 2 commits June 23, 2021 21:20

Merge branch 'dev' of github.com:biocore/qiita into add-default-proce…

8ed770b

…ssing

fix errors

53450db

wasade reviewed Jun 29, 2021

View reviewed changes

antgonza commented Jun 29, 2021

View reviewed changes

Merge branch 'dev' of github.com:biocore/qiita into add-default-proce…

3ea4da7

…ssing

addressing @wasade comments

4a97f92

antgonza commented Jun 30, 2021

View reviewed changes

wasade merged commit ef7f873 into qiita-spots:dev Jun 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add default processing #3116

Add default processing #3116

antgonza commented Jun 24, 2021

coveralls commented Jun 24, 2021 •

edited

Loading

wasade Jun 29, 2021

wasade Jun 29, 2021

wasade Jun 29, 2021

wasade Jun 29, 2021

antgonza Jun 29, 2021

wasade Jun 29, 2021

wasade Jun 29, 2021

antgonza Jun 29, 2021

wasade Jun 29, 2021

wasade Jun 29, 2021

antgonza Jun 29, 2021

wasade Jun 29, 2021

wasade Jun 29, 2021

antgonza Jun 29, 2021

wasade Jun 29, 2021

antgonza Jun 30, 2021

wasade Jun 29, 2021

antgonza Jun 29, 2021

wasade Jun 29, 2021

antgonza Jun 30, 2021

antgonza left a comment

antgonza Jun 29, 2021

antgonza Jun 29, 2021

antgonza Jun 29, 2021

antgonza Jun 29, 2021

antgonza Jun 29, 2021

mestaki commented Jun 30, 2021

antgonza commented Jun 30, 2021

antgonza left a comment

antgonza Jun 30, 2021

antgonza Jun 30, 2021

wasade commented Jun 30, 2021

antgonza commented Jun 30, 2021


		def add_default_workflow(self, user):
		"""The modification timestamp of the prep information

Add default processing #3116

Add default processing #3116

Conversation

antgonza commented Jun 24, 2021

coveralls commented Jun 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antgonza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mestaki commented Jun 30, 2021

antgonza commented Jun 30, 2021

antgonza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wasade commented Jun 30, 2021

antgonza commented Jun 30, 2021

coveralls commented Jun 24, 2021 •

edited

Loading