Implements bulk_create for create_batch if available

[kingbuzzman]

What this PR is trying to accomplish:

• There is a lot of test code linked to a series of nested-russian-doll sub factories of 1-to-3-to-3-to-1 * 10 (create_batch(10)) models that get created constantly throughout our code base.
  • If we can avoid creating all these models one by one, and leverage django's bulk_create, we would increase performance, and most importantly; it would save a lot of time.
• This feature is turned off by default, and can be turned on via Factory._meta.use_bulk_create.
• Right now i'm targetting postgres
  • I became aware recently that newer versions of sqlite support it now too, that would be fine, but not my main concern

Whats left to do:

• [X] Get buy-in from the maintainers
• [x] Fix a lot of the code / clean up -- this is just a demo.
• [x] Write more tests
• [X] ~Fix a weird error in examples.~ NM all green 👍
• [ ] More clean up
• [ ] Write docs
• [X] Works with postgres and >= py3.9 sqlite (possibly a django limitation??)

Depends on:

• https://github.com/FactoryBoy/factory_boy/pull/931

[kingbuzzman]

@francoisfreitag bump

[kingbuzzman]

@francoisfreitag What do you think of separating some of these files into a different PR to make it easier to review later.

• .github/workflows/test.yml
• tox.ini
• tests/djapp/settings_pg.py
• tests/test_django.py (partial)

What do you think?

[francoisfreitag]

Just to confirm, you’re suggesting to open a PR to run the factory boy Django tests against PostgreSQL ? If so, yes, please go ahead. It is an improvement regardless of this feature, it increases the test coverage for the library.

[kingbuzzman]

@francoisfreitag PR https://github.com/FactoryBoy/factory_boy/pull/931 is ready for review. Thanks!

[francoisfreitag]

Hi Javier,

Thanks for splitting it up. I won't be able to take a look at your work before next week. I'll try to review it quickly then.

May 24, 2022 23:22:32 Javier Buzzi @.***>:

@francoisfreitag[https://github.com/francoisfreitag] PR #931[https://github.com/FactoryBoy/factory_boy/pull/931] is ready for review. Thanks!

— Reply to this email directly, view it on GitHub[https://github.com/FactoryBoy/factory_boy/pull/925#issuecomment-1136440936], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAVBMY2FHNUJLGMVXWD7ZF3VLVBOVANCNFSM5VJKD2HQ]. You are receiving this because you were mentioned.[Tracking image][https://github.com/notifications/beacon/AAVBMY6ZOKNK67K3YAOAYOTVLVBOVA5CNFSM5VJKD2H2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOIO6LM2A.gif]

[kingbuzzman]

@francoisfreitag When you can, can you start looking at this, I'm slowly cleaning up the tests. My biggest hurtle is this right here -- I find it nasty / dirty. Let me know your thoughts.

[kingbuzzman]

@thedrow Thank you for the review

[tony]

@francoisfreitag Do you need any help QA'ing or testing this?

[francoisfreitag]

Hi Tony,

Feel free to share any bugs or weird behavior that you find. Because tests aren’t passing, there will certainly be changes to the implementation. When I get some free time, I’ll give it a deeper look. In the meantime, solving the test failures is a very helpful task, to both understand the code and help others understand it.

[tony]

@francoisfreitag @kingbuzzman

Bikeshed: I won't count on it, but it's a long running PR and the commits have built up. It's sometimes easier to read if commits are squashed and it's rebased against master.

I'm not sure if there's a policy on that, or if it's more harm than good, though.

Either way, I backed up 3666f10 on my fork at pr-925-3666f10 in the event it's ever necessary to look back

[kingbuzzman]

@tony everything went awry after https://github.com/FactoryBoy/factory_boy/pull/925/commits/8ec88e944d29abda80281fdc678ba74a31b2c8e9 , going from https://github.com/FactoryBoy/factory_boy/pull/925/commits/4369b72e50ff82faa1d9cc4c65c3e17789d991b2 makes all the tests pass. I'm going to work on this a little today as i have a little time.

@francoisfreitag .. what a mess. :/

[kingbuzzman]

@francoisfreitag the code you gave me to avoid collecting all the created objects instead of my "half-baked" instance level solution does not work. I also don't see it feasible to send the collect_instances down 13 levels to collect the models. Think of a crazy model nesting: A->B <-[C1, C2->D]

What else can we do?

Ps a good test that demonstrates the issue is tests/test_django.py::DependencyInsertOrderTest:test_new_m2m

Stack of how far `created_instances` would need to go (with room to grow)

  /factory_boy/tests/test_django.py(324)test_new_m2m()
    323         created_instances = []
--> 324         a1 = step.build(collect_instances=created_instances)
    325         print('*'*30, created_instances)

  /factory_boy/factory/builder.py(279)build()
    278             declaration = post[declaration_name]
--> 279             postgen_results[declaration_name] = declaration.declaration.evaluate_post(
    280                 instance=instance,

  /factory_boy/factory/declarations.py(608)evaluate_post()
    607         )
--> 608         return self.call(instance, step, postgen_context)
    609 

  /factory_boy/factory/declarations.py(710)call()
    709         parent = super()
--> 710         return [
    711             parent.call(instance, step, context)

  /factory_boy/factory/declarations.py(711)<listcomp>()
    710         return [
--> 711             parent.call(instance, step, context)
    712             for i in range(self.size if isinstance(self.size, int) else self.size())

  /factory_boy/factory/declarations.py(689)call()
    688         )
--> 689         return step.recurse(factory, passed_kwargs)
    690 

  /factory_boy/factory/builder.py(218)recurse()
    217         builder = self.builder.recurse(factory._meta, declarations)
--> 218         return builder.build(
    219             parent_step=self,

  /factory_boy/factory/builder.py(264)build()
    263         )
--> 264         step.resolve(pre)
    265 

  /factory_boy/factory/builder.py(201)resolve()
    200         for field_name in declarations:
--> 201             self.attributes[field_name] = getattr(self.stub, field_name)
    202 

  /factory_boy/factory/builder.py(353)__getattr__()
    352                 try:
--> 353                     value = value.evaluate_pre(
    354                         instance=self,

  /factory_boy/factory/declarations.py(48)evaluate_pre()
     47         context = self.unroll_context(instance, step, overrides)
---> 48         return self.evaluate(instance, step, context)
     49 

  /factory_boy/factory/declarations.py(411)evaluate()
    410         force_sequence = step.sequence if self.FORCE_SEQUENCE else None
--> 411         return step.recurse(subfactory, extra, force_sequence=force_sequence)
    412 

  /factory_boy/factory/builder.py(218)recurse()
    217         builder = self.builder.recurse(factory._meta, declarations)
--> 218         return builder.build(
    219             parent_step=self,

> /factory_boy/factory/builder.py(276)build()
    275 
--> 276         postgen_results = {}
    277         for declaration_name in post.sorted():

[kingbuzzman]

@francoisfreitag bump

[francoisfreitag]

I’ll be busy until the end of the month / year. I’ll do my best to carve out some time for this issue, but the ask if not for a few minutes, it’ll take much longer to find a correct integration of the collected objects in the existing machinery.

[tony]

Is there going to be a rebase / squash?

[kingbuzzman]

@tony does it matter at the moment? Rather keep the history at this time in case I need to bring something back. I'll squash as this comes closer to finish

[tony]

@kingbuzzman No worries! Whatever works best for you.

To minimize noise, I'll stay out of the way unless you / another maintainer requests a review (earlier mention).

(Fine to minimize / hide this comment)

[kingbuzzman]

@tony last I saw the tests were passing... weird. If you want to take it out for a spin, you're more than welcome!

Any changes you want you can make PRs over at my fork and I'll merge them in so it can be shown here.

[kingbuzzman]

@francoisfreitag any input?

[francoisfreitag]

I don’t have much time to dedicate to open-source these days, I cannot spend time on this PR for now.

The idea sure has merits, but it needs efforts to get the approach right. My understanding is that https://github.com/FactoryBoy/factory_boy/pull/925#issuecomment-1290292111 needs solving, and without spending some time figuring out a better idea, I won’t be able to make suggestions.

[tony]

@kingbuzzman Thanks for keeping this branch alive!

[kingbuzzman]

I honestly don't understand why it passed 3 "merge 'master' into master" ago, and now its failing, nothing has changed..