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:

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

Depends on:

• Adds postgres tests #931

[kingbuzzman]

@francoisfreitag bump

[francoisfreitag]

Refs #107. The main reason this feature was not implemented seems to be the lack of motivated developers.

Django bulk_create has several documented limitations, but none seem deal breakers.

The opt-in option is probably the best way to achieve backward compatibility and incremental adoption. Also gives an escape hatch for bulk_create limitations.

I see no reason to reject the PR, provided the code gets to a mature state and we can get CI on it and good test coverage. I’m actually pretty excited about it.

The idea of collecting "leaf" models and working up the insert chain should work, though we need to be careful of post generation hooks. Perhaps they should be documented as incompatible with bulk_create(), at least as a first step.

I’m not eager to be introducing more DB-specific code in factory_boy, but that seem unavoidable with this feature and can rely on Django attributes to abstract some of the difficulties.

This patch inspects Django models to describe the relationship between objects, where factory_boy is mostly declarative (SubFactory, RelatedFactory and co). I don’t foresee conflicts arising from that yet, and using Django as the source of truth is probably safe.

[thedrow]

Overall this looks great! I wish I had time to do this myself 8 years ago.

I have a few rather minor comments.

[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.

[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 8ec88e9 , going from 4369b72 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 #925 (comment) 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..

[jurrian]

What is the progress on this? I appreciate the introduction of the true bulk create.
Any help needed at this point?

[cameron-hobbs]

I am also keen for bulk create implementation to be introduced - glad to help

[ulgens]

It looks like something went wrong with the PR and there are lot of unrelated stuff in it. A rebase would be nice but at this point, recreating it will probably be cleaner.

(Merging from main branch into feature branches generally ends like similar results to this one.)

I'd also recommend that instead of using the main branch in your fork, creating a PR from a feature branch to upstream main gives better/cleaner results.

[kingbuzzman]

I'll rebase it, i dont want to lose the history. I need to fix the tests when i get a chance. I've been really busy with the day job 😇