Filtering the batch based on model type #3756
Conversation
|
Hello @daviddavis! Thanks for submitting the PR.
|
Codecov Report
@@ Coverage Diff @@
## master #3756 +/- ##
=======================================
Coverage 54.77% 54.77%
=======================================
Files 64 64
Lines 2806 2806
=======================================
Hits 1537 1537
Misses 1269 1269Continue to review full report at Codecov.
|
| @@ -52,7 +52,8 @@ class QueryExistingContentUnits(Stage): | |||
|
|
|||
| for model_type in content_q_by_type.keys(): | |||
| for result in model_type.objects.filter(content_q_by_type[model_type]): | |||
| for declarative_content in batch: | |||
| model_batch = filter(lambda dcont: type(dcont.content) == model_type, batch) | |||
There was a problem hiding this comment.
I only provided the batch iterator for this code, so I am not the expert here.
These nested loops look like they have the word "SLOW" imprinted on them (but let's wait for the outcome of the performance testing).
Given the current structure, it looks like the right thing to do. However, I would prefer staying imperative style here. There is already a continue in this loop, so I would prefer:
for declarative_content in batch:
if type(declarative_content.content) is not model_type:
continue
not_same_unit = False
...An alternative would be to sort the batch into buckets by content type in the loop above (analogous to how the query is built) and iterate through the specific bucket here.
There was a problem hiding this comment.
+1 to the code change suggestion for readability. In terms of performance we'll need to improve it over time along with functional and unit tests to ensure we don't regress along the way.
|
@daviddavis thank you for working on this. |
I'm pretty sure this is probably not the right fix. @gmbnomis or @bmbouter, hoping you can suggest what to do.
fixes #4165
https://pulp.plan.io/issues/4165