Fix 1812 #2371

antgonza · 2017-10-21T19:10:19Z

No description provided.

codecov-io · 2017-10-22T13:34:30Z

Codecov Report

Merging #2371 into dev will decrease coverage by 0.01%.
The diff coverage is 86.88%.

@@            Coverage Diff             @@
##              dev    #2371      +/-   ##
==========================================
- Coverage   93.78%   93.76%   -0.02%     
==========================================
  Files         163      163              
  Lines       18615    18643      +28     
==========================================
+ Hits        17458    17481      +23     
- Misses       1157     1162       +5

Impacted Files	Coverage Δ
qiita_db/handlers/processing_job.py	`96.92% <ø> (ø)`	⬆️
qiita_db/analysis.py	`95.96% <ø> (ø)`	⬆️
...t/handlers/api_proxy/tests/test_sample_template.py	`98.55% <ø> (ø)`	⬆️
qiita_pet/handlers/study_handlers/ebi_handlers.py	`62.5% <0%> (ø)`	⬆️
qiita_pet/handlers/api_proxy/studies.py	`86.66% <0%> (ø)`	⬆️
...iita_pet/handlers/study_handlers/vamps_handlers.py	`21.21% <0%> (ø)`	⬆️
qiita_pet/handlers/api_proxy/sample_template.py	`95.69% <100%> (ø)`	⬆️
qiita_pet/handlers/api_proxy/prep_template.py	`90.04% <100%> (ø)`	⬆️
...ta_pet/handlers/artifact_handlers/base_handlers.py	`93.33% <100%> (ø)`	⬆️
qiita_db/processing_job.py	`92.75% <100%> (+0.21%)`	⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fe81b0...aa27112. Read the comment docs.

antgonza · 2017-10-22T13:51:50Z

This is ready for review: @josenavas, @ElDeveloper and @wasade. I think 2-3 reviews will be great. The main things to consider while reviewing:

The ProcessingJob.create has a new parameter force to force creation, as in some cases is fine to force creation even with duplicated parameters.
Be careful to check that it makes sense to use that flag in the tests, for example: must of our tests create duplicated jobs and @josenavas and I thought this was fine
Sample/Prep/Analysis workflows can still create duplicated jobs, thus adding there.

josenavas

Just one comment

josenavas · 2017-10-23T14:15:23Z

qiita_db/processing_job.py

+            existing_jobs = qdb.sql_connection.TRN.execute_fetchindex()
+            if existing_jobs and not force:
+                raise ValueError(
+                    'Cannot create job because these jobs:\n%s' % '\n'.join(


"Cannot create job because it is a duplicate of these jobs"

👍 ... maybe worth including a bit more detail:

Cannot create job because the parameters are the same as jobs that are queued, running or already have succeeded: .....

ElDeveloper

Just one comment in the double list comprehension.

ElDeveloper · 2017-10-23T16:28:16Z

qiita_db/processing_job.py

+                     WHERE command_id = %s AND processing_job_status IN (
+                        'success', 'waiting', 'running') """
+            params = " AND ".join(
+                ["command_parameters->>'%s' = '%s'" % (k, v)


This is one confusing double conditional list comprehension. for v in v?

Thanks, fixing. Note that this wasn't actually working correctly and I also added some explanation on the steps. Let's see what else this breaks.

ElDeveloper · 2017-10-23T16:34:41Z

qiita_db/processing_job.py

+            existing_jobs = qdb.sql_connection.TRN.execute_fetchindex()
+            if existing_jobs and not force:
+                raise ValueError(
+                    'Cannot create job because these jobs:\n%s' % '\n'.join(


👍 ... maybe worth including a bit more detail:

Cannot create job because the parameters are the same as jobs that are queued, running or already have succeeded: .....

wasade · 2017-10-23T17:30:01Z

qiita_db/processing_job.py

+                     WHERE command_id = %s AND processing_job_status IN (
+                        'success', 'waiting', 'running') """
+            params = " AND ".join(
+                ["command_parameters->>'%s' = '%s'" % (k, v)


wasade · 2017-10-23T17:31:03Z

qiita_db/test/test_processing_job.py

@@ -22,7 +22,7 @@
 from qiita_core.qiita_settings import qiita_config


-def _create_job():
+def _create_job(force=True):


If we default to True, are there situations in the codebase where this is False?

Or rather, this API has changed I believe since the existing functionality would be force=False if I understand the docs above. Will this change be a problem elsewhere?

...or was the default behavior previously to force the execution?

This is a local function to avoid duplication of code that by default always ignores/forces the duplicated jobs. There is a single test (below) that doesn't force this behavior: _create_job(False)

josenavas · 2017-10-23T21:59:44Z

@antgonza you've test failures here

antgonza · 2017-10-23T22:38:18Z

Don't merge this as it still needs some work. The code works as expected but the GUI is not catching/working as expected.

antgonza · 2017-10-23T23:17:47Z

Well, the "fix" was easier than expected, the error now is shown like this:

antgonza · 2017-10-23T23:30:22Z

@wasade and @ElDeveloper, could you do another pass? Thanks!

ElDeveloper · 2017-10-24T00:58:28Z

qiita_db/processing_job.py

+            for k, v in viewitems(parameters.values):
+                # this is necessary in case we have an Iterable as a value
+                # but that is not unicode or string
+                if isinstance(v, Iterable) and not (isinstance(v, str) or


isinstance can take a tuple i.e. isinstance(v, (str, unicode)).

k, changing

ElDeveloper · 2017-10-24T01:01:59Z

qiita_db/test/test_processing_job.py

+        with self.assertRaisesRegexp(ValueError, 'Cannot create job because '
+                                     'the parameters are the same as jobs '
+                                     'that are queued, running or already '
+                                     'have succeeded:'):


How is this passing if this is not a regex, and there's no pattern to match here? Shouldn't there be a job id, or a regex indicating that this should match a job id?

cause it behaves as an IN.

antgonza added 6 commits October 21, 2017 12:54

fix qiita-spots#1812

91b9e08

add tests

35b0168

flake8

352e9bb

populating ProcessingJob.create True

0da47b2

fix more errors

0dc98c6

mv ProcessingJob.create True around

180b6d7

josenavas approved these changes Oct 23, 2017

View reviewed changes

ElDeveloper suggested changes Oct 23, 2017

View reviewed changes

antgonza added 2 commits October 23, 2017 10:52

Merge branch 'dev' of https://github.com/biocore/qiita into fix-1812

03db962

addressing @ElDeveloper and @josenavas comments

1fb126d

wasade requested changes Oct 23, 2017

View reviewed changes

addressing @wasade and @ElDeveloper comments

7303611

fixing errors

123ef94

antgonza changed the title ~~Fix 1812~~ [WIP] Fix 1812 Oct 23, 2017

fix GUI and erros

c5db978

antgonza changed the title ~~[WIP] Fix 1812~~ Fix 1812 Oct 23, 2017

ElDeveloper suggested changes Oct 24, 2017

View reviewed changes

addressing @ElDeveloper comments

aa27112

ElDeveloper approved these changes Oct 24, 2017

View reviewed changes

wasade merged commit 66e3b09 into qiita-spots:dev Oct 24, 2017

antgonza mentioned this pull request Oct 24, 2017

don't allow resubmission of same jobs #1812

Closed

antgonza deleted the fix-1812 branch February 17, 2018 15:04

Fix 1812 #2371

Fix 1812 #2371

Uh oh!

Conversation

antgonza commented Oct 21, 2017

Uh oh!

codecov-io commented Oct 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

antgonza commented Oct 22, 2017

Uh oh!

josenavas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ElDeveloper left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josenavas commented Oct 23, 2017

Uh oh!

antgonza commented Oct 23, 2017

Uh oh!

antgonza commented Oct 23, 2017

Uh oh!

antgonza commented Oct 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-io commented Oct 22, 2017 •

edited

Loading