Add Async Factory Support #803

nadege · 2020-10-30T17:26:58Z

Hello,

I heard you may be interested in a PR to add support for async Factories, related issue : #679

We use the solution I documented in the issue in a project at work (aiohttp + aiopg) and it works well,
but even if we have models with some complexity (lot of Subfactory themselves using SelfAttribute),
I know we don't use a lot of features of Factory Boy...

For this PR I chose to add a new strategy for async creation, and added a new class AsyncFactory that default to this strategy.
I also added a few methods to class Factory, and functions to the helpers module,
to create, batch create, generate, batch generate... with this new strategy.
There should be no issue with retro-compatibility, it only add new features.

I wrote some tests and documentation but they may be insufficient.
Advice on what to test or document further is welcome of course.

Duplicating all tests to run them with an async factory seems a bit overkill to me,
but maybe we should for some test cases ?

what do you think ?

francoisfreitag

Hi @nadege,

Thanks a lot for taking the time to prepare this patch 🎉.

Async capabilities in factory_boy seems to be interesting to a portion of the user base and would benefit the library. Django is planning to eventually support creating objects asynchronously and SQLAlchemy has GINO or aiopg, movement in that direction is great.

Thanks for adding tests following the create methods, it helped review a lot. The suggested changes make sense to me, though I am not very familiar with async in Python.

~~Can tests be run with the Python development mode to catch potential issues in asyncio?~~ Enabled in #812.

The new extension points should be listed in the extension points section https://github.com/FactoryBoy/factory_boy/blob/9b256e48dd58ef7c97e3ddf09d972d1bb1d2df8e/docs/reference.rst.

An entry should be added to the release note.

factory/helpers.py

tests/test_using.py

factory/base.py

tests/test_base.py

tests/test_using.py

docs/introduction.rst

factory/base.py

nadege · 2020-11-04T15:56:17Z

Thanks for the review!
I'll fix the simpler things first (doc, tests..) and then try to add an implementation of _create_model_async for a library.

nadege · 2020-11-27T16:20:24Z

I added an implementation for https://github.com/encode/orm ORM in the last commit.
It's the best I found which support async and sqlite.
I don't know how to simply add a postgresql database to the current CI,
but when it'll be replaced by github actions it should be easier.
I'll work on an implementation for a PG backed ORM then.

Last problem: unittest.IsolatedAsyncioTestCase is new in python3.8
I need to find a solution for previous python versions.

(I also have documentation spelling errors.)

nadege · 2020-12-04T16:52:40Z

I resolved the compatibility issues with older python versions and spelling errors in the doc.
Should be ready for a new review

nadege · 2020-12-05T11:39:39Z

oh I still need to update /docs/reference.rst, the release note and CREDIT files.

nadege · 2020-12-05T13:11:55Z

I'll wait for djangoproject.com to be fixed so I can run make doc before pushing my next commit.
Also that's why CI failed on my last commit.

francoisfreitag

I took a quick look at it today, it seems great! 🎉
I’ll try to dive deeper later this week. Thanks for adding support for SQLite, it helps exercising the new code and increases confidence in the patch.

I don't know how to simply add a postgresql database to the current CI, but when it'll be replaced by github actions it should be easier.

https://docs.travis-ci.com/user/database-setup/#postgresql should be helpful. If that does not work out, I’m happy with the SQLite version to start with. The more test coverage, the better, but IMO PostgreSQL support is not mandatory for landing this patch. Getting started on async benefits the project and we can increase test coverage as we go.

docs/introduction.rst

factory/base.py

tests/test_orm.py

tox.ini

tests/test_using.py

nadege · 2020-12-11T15:08:41Z

Issues highlighted in previous review should be fixed. Added doc, credits and changelog.
Ready for next review :)

francoisfreitag

I wonder how async factories relate to post generation hooks https://factoryboy.readthedocs.io/en/latest/reference.html#postgeneration?

The post generation hooks assume the instance was created by the call to instantiate:

factory_boy/factory/builder.py

Lines 268 to 272 in 88b81ea

    
           instance = self.factory_meta.instantiate( 
        
               step=step, 
        
               args=args, 
        
               kwargs=kwargs, 
        
           )

Since create_async returns a task and not an instance, perhaps the post generation hooks should await it before to run? Or register the post generation hooks as add_done_callback on the Task?
Or do you expect subclasses of AsyncFactory to expect the task as the first positional argument, awaiting it if necessary?

docs/changelog.rst

docs/introduction.rst

factory/base.py

tests/test_base.py

francoisfreitag · 2020-12-17T20:12:30Z

tests/test_using.py

+    def test_create_async(self):
+        loop = asyncio.get_event_loop()
+        obj = loop.run_until_complete(factory.create_async(FakeAsyncModel, foo='bar'))
+        self.assertEqual(obj.id, None)


id is None because create() was not called.

Is that the expected behavior? I would expect that when the task to create the object is done, the object has been recorded to the database and has an id?

It's expected because the make_factory function will output a factory based on Factory class.
So we end up with model_class(**kwargs) to create the object, not the model async create function.
I'll add comments to the test.

For comparison, the sync test:

def test_create(self): obj = factory.create(FakeModel, foo='bar') self.assertEqual(obj.id, None) self.assertEqual(obj.foo, 'bar')

If you look at the test test_create_async_custom_base, it has parameter FACTORY_CLASS=FakeAsyncModelFactory and the async creation is ok.

factory/helpers.py

docs/introduction.rst

TheLazzziest · 2020-12-26T17:06:12Z

Hi, @nadege . Great work you've done. However, I would like to ask will you plan to add async support for post_generation methods ? I haven't found any code changes related to that feature.

nadege · 2021-01-15T09:26:07Z

Hello !
Ok I found time to continue work on this. I'll look into the post generation feature. I think I never used that before.

nadege · 2021-01-15T16:15:04Z

I handled the remarks you made in the last review. Now I'll look into post-generation

mdczaplicki · 2021-01-18T14:40:53Z

Seems that LazyAttribute that depends on an async SubFactory receives a Task instead of an object.

nadege · 2021-01-22T10:06:08Z

@mdczaplicki would you have a test that reproduce the issue ?

mdczaplicki · 2021-01-22T15:37:38Z

Sure, I'll provide you with something next week. :)

…

On Jan 22 2021, at 11:06 am, Nadege Michel ***@***.***> wrote: @mdczaplicki (https://github.com/mdczaplicki) would you have a test that reproduce the issue ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub (#803 (comment)), or unsubscribe (https://github.com/notifications/unsubscribe-auth/ACFPY2QYTE2IMOMIRBU7NE3S3FE2DANCNFSM4TFHREYQ).

fadedDexofan · 2021-02-21T12:43:58Z

@nadege @mdczaplicki Any progress on this?

nadege · 2021-02-24T11:00:58Z

No, I'm between jobs and my personal computer just broke, so I can't work on this at the moment.

romulorosa · 2021-06-30T11:32:43Z

Hey @nadege the work here is awesome! 🥇 I am really interested on having this feature kicked out of the door, do you have any plans to finish it? If you don't mind I would be happy to jump in and help to get it done. Could you tell what is missing?

francoisfreitag · 2021-06-30T11:50:07Z

IIRC, the main piece remaining is deciding how to handle the post-generation hooks.

nadege · 2021-06-30T13:23:12Z

hi @romulorosa ! I don't have much time (or energy :x) to continue this short term. So I'd be happy to see you take it from here !

gegnew · 2022-11-01T17:06:41Z

any updates on this PR?

ShipraShalini · 2022-11-04T11:42:50Z

IIRC, the main piece remaining is deciding how to handle the post-generation hooks.

@francoisfreitag Can this feature be released in parts? As in, first release the basic functionality and then add the post-generation-hooks in a future release.

francoisfreitag · 2022-11-04T11:58:40Z

The post generation hooks need handling. The handling can probably be to raise NotImplemented when a post generation hook is declared on an AsyncFactory. Although clearly not ideal, that would at least improve the use case for factory boy in an async context.

ShipraShalini · 2022-11-04T15:56:41Z

My use case doesn't require post-generation-hooks, so I can work on getting it out with raise NotImplemented.

coneybeare · 2023-01-05T16:19:28Z

I'd like to see this PR get merged too. @ShipraShalini did you make some progress there on the raise NotImplemented ? Can I offer assistance?

nadege · 2023-07-22T10:46:03Z

Hello there, I'm back 🙇

Changes on the PR since last time (3 years ago, time flies... 👵)

😅 Rebase
🔄 Replaced orm implementation by a SQLAlchemy one (with sqlite only)
🔥 Dropped python 3.6 support. Now using asyncio.run.
🛠️ Fix lint & other CI failures

TODO:

🧪 One test for Post Generation hook.
❓ Decide what to do about post generation. (NotImplemented exception, handle it...)
🧪 More tests? (please advise!)

Dropped:

async implem in sqlalchemy app works with postgres. I tried to use asyncpg, the test were stuck in "teardown" of asyncs tests in test_alchemy.py.

nadege · 2023-07-23T10:47:10Z

And, post generation is now handled too! 🎉

I played with the suggestion from @francoisfreitag, to use add_done_callback.
Now the whole post generation step is a callback when dealing with a Task.

Code is in last commit, it's a bit quick & dirty. Not sure of the cleanest way to declare the _handle_post_generation function.
Happy to make any change or add more tests. Please advise 🙇

matinone · 2023-12-05T22:40:35Z

This is awesome @nadege. I hope it gets merged soon, it's been a few months already since your last commit.

mlorenc-sonnen · 2023-12-06T14:22:52Z

Great work @nadege , it was very helpful in fixing some of the issues with the solution in my team. I also hope this gets merged soon so we can migrate away from temporary workarounds :)

nextmat · 2024-01-04T17:46:11Z

Appreciate all the work on this @nadege. :-)

@francoisfreitag is there other work needed to get this merged? Thanks!

francoisfreitag

Great work overall, thank you!! ⭐ 🙌

I believe this is ready for merging, pending some minor RST fixes and maybe a couple of test tightening. I’ll try to push a few commits in the upcoming weeks, depending on my newborn’s sleep.
Feel free to beat me to it, if you have time before then!

Pinging @rbarrois, in case you would like to review this work before inclusion.

factory/base.py

docs/reference.rst

docs/introduction.rst

docs/reference.rst

tests/test_alchemy.py

tests/test_using.py

nadege · 2024-01-13T10:56:10Z

factory/alchemy.py

+    @classmethod
+    async def _save(cls, model_class, session, args, kwargs):
+        session_persistence = cls._meta.sqlalchemy_session_persistence

-            return model
+        obj = model_class(*args, **kwargs)
+        session.add(obj)
+        if session_persistence == SESSION_PERSISTENCE_FLUSH:
+            await session.flush()
+        elif session_persistence == SESSION_PERSISTENCE_COMMIT:
+            await session.commit()
+        return obj


Code was not actually saving the object to the db. 😬

TODO:

add a test that would have shown that

handle get_or_create in async create as well, or can we ship without? (I won't have bandwidth for it)

nadege · 2024-01-13T10:59:16Z

I rebased & addressed the simpler comments, realised we were not committing/flushing so I fixed that.

TODO:

test that we actually save objects in slqalchemy implementation
use IsolatedAsyncioTestCase in tests
handle get_or_create in async implem for sqlalchemy?

I can find time for first 2 items.

Edit: Thank you and congrats for the baby! 😁

rushilsrivastava · 2024-07-18T01:04:48Z

Hey @nadege! How can I help move this PR along? I would love to see this merged in :)

I am happy to tackle the get_or_create async implementation

nadege force-pushed the async-factories branch from 0303291 to 077c598 Compare October 30, 2020 17:33

francoisfreitag reviewed Nov 2, 2020

View reviewed changes

nadege force-pushed the async-factories branch 2 times, most recently from 780909f to 5f0c9f6 Compare November 27, 2020 16:14

nadege force-pushed the async-factories branch from 5f0c9f6 to 330c5e9 Compare December 4, 2020 16:24

francoisfreitag reviewed Dec 6, 2020

View reviewed changes

tests/test_using.py Outdated Show resolved Hide resolved

nadege force-pushed the async-factories branch 3 times, most recently from 78e378f to b005370 Compare December 11, 2020 14:58

francoisfreitag reviewed Dec 17, 2020

View reviewed changes

nadege force-pushed the async-factories branch 2 times, most recently from bb0d472 to 9d94c4f Compare January 15, 2021 16:11

nadege force-pushed the async-factories branch from 9d94c4f to 8ea8f73 Compare July 22, 2023 10:42

nadege force-pushed the async-factories branch 3 times, most recently from 03dc0f2 to e282b2a Compare July 23, 2023 08:50

francoisfreitag reviewed Jan 11, 2024

View reviewed changes

nadege and others added 6 commits January 13, 2024 09:11

Documentation, changelog and credits

abfc39e

Add async Factory support

2607f5c

Add test for async factories

e66dce0

Add Async Factory implementation for SqlAlchemy

bcbbba0

Add support for post generation in async creation

705b6ff

Rebased and adresses simple comments

5f2582d

nadege force-pushed the async-factories branch from 054a945 to 5f2582d Compare January 13, 2024 10:06

Alchemy: Actually save the objects in db

4ece4ba

nadege commented Jan 13, 2024

View reviewed changes

	instance = self.factory_meta.instantiate(
	step=step,
	args=args,
	kwargs=kwargs,
	)

Add Async Factory Support #803

Are you sure you want to change the base?

Add Async Factory Support #803

Conversation

nadege commented Oct 30, 2020

francoisfreitag left a comment • edited Loading

Choose a reason for hiding this comment

nadege commented Nov 4, 2020

nadege commented Nov 27, 2020 • edited Loading

nadege commented Dec 4, 2020

nadege commented Dec 5, 2020

nadege commented Dec 5, 2020

francoisfreitag left a comment

Choose a reason for hiding this comment

nadege commented Dec 11, 2020

francoisfreitag left a comment

Choose a reason for hiding this comment

francoisfreitag Dec 17, 2020

Choose a reason for hiding this comment

nadege Jan 15, 2021

Choose a reason for hiding this comment

TheLazzziest commented Dec 26, 2020

nadege commented Jan 15, 2021

nadege commented Jan 15, 2021

mdczaplicki commented Jan 18, 2021

nadege commented Jan 22, 2021

mdczaplicki commented Jan 22, 2021 via email

fadedDexofan commented Feb 21, 2021

nadege commented Feb 24, 2021

romulorosa commented Jun 30, 2021

francoisfreitag commented Jun 30, 2021

nadege commented Jun 30, 2021

gegnew commented Nov 1, 2022

ShipraShalini commented Nov 4, 2022

francoisfreitag commented Nov 4, 2022

ShipraShalini commented Nov 4, 2022

coneybeare commented Jan 5, 2023

nadege commented Jul 22, 2023 • edited Loading

nadege commented Jul 23, 2023

matinone commented Dec 5, 2023

mlorenc-sonnen commented Dec 6, 2023

nextmat commented Jan 4, 2024

francoisfreitag left a comment

Choose a reason for hiding this comment

nadege Jan 13, 2024

Choose a reason for hiding this comment

nadege commented Jan 13, 2024 • edited Loading

rushilsrivastava commented Jul 18, 2024

francoisfreitag left a comment •

edited

Loading

nadege commented Nov 27, 2020 •

edited

Loading

nadege commented Jul 22, 2023 •

edited

Loading

nadege commented Jan 13, 2024 •

edited

Loading