Graphene-django: ☂️Graphene-Django v3

[jkimbo]

This issue is to track v3 of Graphene-Django which will contain some breaking changes.

WIP branch: v3 (compare)

Breaking changes

• [x] Upgrade to Graphene v3 (which also upgrades graphql-core to v3) (PR work in progress: https://github.com/graphql-python/graphene-django/pull/905)
• [x] Convert MultipleChoiceField to List of type String (#611)
• [x] Start raising DeprecationWarnings for using only_fields and exclude_fields (see https://github.com/graphql-python/graphene-django/pull/691) (PR #980)
• [x] Start warning if neither fields or exclude are defined on DjangoObjectTypes https://github.com/graphql-python/graphene-django/issues/710 (PR #981)
• [x] Default CAMELCASE_ERRORS setting to True (PR #789)
• [x] Rename DJANGO_CHOICE_FIELD_ENUM_V3_NAMING to DJANGO_CHOICE_FIELD_ENUM_V2_NAMING and default it to False (reference #860) (PR #982)
• [x] Convert decimal fields correctly: https://github.com/graphql-python/graphene-django/issues/91

TODO

• [ ] Check dependant libraries: https://github.com/tfoxy/graphene-django-optimizer and https://github.com/eamigo86/graphene-django-extras

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[ktosiek]

Hey stale bot, this issue is not dead yet, it's just getting started!

[ktosiek]

Has anyone started working on this? I'm currently trying to just make tests pass under graphene 3.

[swist]

Is there an up to date todolist on this? I have a couple of days this week so can pick it up and finish off your branch

[ktosiek]

I think it's complete enough to try building on top of it (porting an actual application, or checking if graphene-django-optimizer works).
It would also be nice to drop the dependency on promise.

[patrick91]

@ktosiek we could try with https://github.com/graphql-python/swapi-graphene

I wanted to test graphene 3 at work, but we are still on v1 :D

[swist]

I've just tried it on my current project. Seems to work fine minus some of our custom graphene types that allow us to use django_more/django_enum

[BfutureP]

graphene-django doesn;t support graphql 3, says from graphql import get_default_backend not found

[garimagupta715]

is asgi server supported in graphene. how can we implement subscription with graphene

[ulgens]

Any progress on this?

[ganwell]

@ulgens Last month I started to run graphen-django v3 against our test-suite (https://github.com/projectcaluma/caluma), but I got distracted by other projects. I was able to reduce errors from 500 of 600 to about 150 of 600. I assume there are still bugs in the graphql/graphene projects, some are in caluma itself of course. I hope I can resume the work in February.

[ulgens]

@ganwell Thanks for answering to both of my comments :slightly_smiling_face: Is there any way to push your work, so we can continue on that?

[ulgens]

Or maybe, somehow, we can do some partial / incremental changes? I think it'd be more productive and beneficial for everyone.

[ganwell]

@ulgens

I did two things:

1. Match the revisions of all projects to minimize the amount of inconsistencies and give me a base to work without confusing errors because of version mismatches, this was quite a task: https://github.com/ganwell/gwork

2. One pull request with an actual bug: https://github.com/graphql-python/graphene/pull/1111

I want to continue working with the versions in gwork till I found all bugs I can with these versions and the I would iterate and update the versions.

[ulgens]

Hey everyone, i used existing PR and solved all left errors: https://github.com/graphql-python/graphene-django/pull/905

[ulgens]

Hey @jkimbo, for DJANGO_CHOICE_FIELD_ENUM_V3_NAMING settings, instead of defaulting it to True i think this setting should be removed and instead, we should intruduce a new one for V2_NAMING to revert changed behaviour.

[jkimbo]

@ulgens that sounds like a good idea!

[ganwell]

Hey everyone, i used existing PR and solved all left errors: #905

@ulgens Thanks a lot. I currently have the problem that my work is based on #774 and yours is not. I don't know how to proceed at the moment.

caluma test-suite:

355 failed, 222 passed, 96 errors
281 failed, 296 passed, 96 errors
163 failed, 414 passed, 96 errors
161 failed, 500 passed, 12 errors
Switch: 342 failed, 319 passed, 12 errors

I am more or less back where I started. I guess I will continue on #774 and check for every bug if you have already fixed it.

[ulgens]

@ganwell There is small issue with commits in my branch, i can fix it tomorrow. There is no failing test in it, so i think we can continue from there.

[ganwell]

@ulgens Thanks a lot. Could you give me the versions of graphql-core, graphql-relay-py, graphene and graphene-django you are currently using? pip freeze

If you haven't read it in my issue, I am doing an integration test by running 3+ versions of graphql-core, graphql-relay-py, graphene and graphene-django against the caluma test-suite. Of course this will find problems in caluma, but also bugs in the other projects.

So I try not change versions of these projects too often, or am set back every-time, but I guess it would be a good idea to switch to your versions.

[ulgens]

@ganwell All versions can be found in related PR. They were all the latest ones when the time of that PR.

[ganwell]

@jkimbo @ulgens I think we need a branch or a repo where we can do a regular review-process for graphene-django-3.0. I spent the whole day yesterday, incrementally merging all the commits from @ktosiek and @ulgens onto current master, and either way there are issues left. I think we need:

• Start from master
• EDIT: Switch to the 3.0+ dependencies
• Mark all CI builds as allow_failures failing tests
• Create small Pull-Requests gradually moving towards using graph*-3.0+ (and remove marks from fixed tests)
• Review these
• Follow changes on master, so we can cleanly merge the branch
• Merge to master
• Release

I am not the right person to review on graphene-django, but if no-one else has the time to do it, I will try.

[jkimbo]

@ganwell yeah I think you're right. How about creating a v3 branch, merging https://github.com/graphql-python/graphene-django/pull/904 then https://github.com/graphql-python/graphene-django/pull/905 and all the related changes for v3. Then we can promote that branch to master once Graphene v3 is out and release a beta of Graphene-Django.

Sound like a good plan @ganwell @ulgens ?

[ulgens]

@jkimbo @ganwell I couldn't catch why the things @ganwell recommended is needed. Everything was already done in #905 I'll check the test-tox config there when i have time but i don't see any further work needed at this point.

[ulgens]

Oh okay, i see what you recommend in that last comment @jkimbo, sorry. I think it makes sense.

[ganwell]

@ulgens @jkimbo I sometimes commented on the wrong issue/PR, so here is a status update:

• @ulgens can fix the problem I spotted easly and #905 will be made ready for merge

• I will find out why the tests are flaky for me and I'll make a pull-request to master in case it is really test order, as I suspect.

[jkimbo]

Thanks @ganwell . I don't understand why the tests on master are failing currently so any help there would be great.

Also @ganwell and @ulgens I've just created a new branch v3 and I've merged the Python 3 changes into it (#904 ). Once you've fixed up #905 @ulgens we can merge that in as well and release a beta version to flush out any other issues.

[jkimbo]

@ganwell did you get anywhere with trying to figure out why the tests are failing on master?

[jkimbo]

Managed to fix one of the failing tests and deleted the other one because I couldn't figure out what was going wrong: https://github.com/graphql-python/graphene-django/pull/931

Bit of a hack but it lets me release a bugfix for https://github.com/graphql-python/graphene-django/issues/922

[ganwell]

@jkimbo I meant that the tests fail if you run only some pytest -k tests.

I created #932

[ganwell]

I will be away from 20 til 26 of April. If still needed I will continue to work on the v3-branch from 27 of April on. @jkimbo Thanks for all the help!

[jkimbo]

@ganwell @ulgens I've just added you both to the repo with write access because of all the great work you've been doing. Would appreciate any help as well!

[ganwell]

@jkimbo Thanks a lot. I'd be glad to help. I will keep an eye on the Issues/PRs. Feel free request help. Of course I will continue to push graph* 3.0 in caluma and do PRs if issues arise.

[jkimbo]

Released the first beta version of v3: https://github.com/graphql-python/graphene-django/releases/tag/v3.0.0b1

Still needs some more changes that are listed in the description but please try it out @ganwell and report back with any issues.

[ganwell]

@jkimbo I found some time to run the caluma test-suite:

170 failed, 505 passed, 71 errors in 21.58s

This means it only slightly worse than #774. If update the snapshot tests:

80 failed, 595 passed, 71 errors

I'll start fixing ...

[jkimbo]

v3.0.0b3 released. Thanks @DoctorJohn and @radekwlsk !

[radekwlsk]

@jkimbo What's there left TODO for v3 release? Maybe I can help a bit more.

[jkimbo]

The main blocker is getting graphene v3 released actually: https://github.com/graphql-python/graphene/issues/1127

We also need some release notes for graphene-django. Is that something you could help with @radekwlsk ?

[ganwell]

My next window to work on v3 is in two weeks.

[radekwlsk]

We also need some release notes for graphene-django. Is that something you could help with @radekwlsk ?

I suppose it should be based in the list at the top of this issue and diff from master? Sounds like something I could help with, at least creating a draft for it.

[jkimbo]

That would be very helpful thanks @radekwlsk

[jkimbo]

v3 branch updated to latest master

[fabienheureux]

I fixed a few things in graphene-django-extras to bring graphene v3 compatibility, if ever some of you are using it, I would greatly appreciate your feedback on this PR https://github.com/eamigo86/graphene-django-extras/pull/148

[patrys]

I'm trying to get DjangoConnectionField to work in my project, it looks like graphene.relay.connection.IterableConnectionField.wrap_resolve replaces what DjangoConnectionField.get_resolver returns which causes DjangoConnectionField.connection_resolver to be called with just two arguments (and complain about missing arguments).

graphene = "3.0.0b5"
graphene-django = "3.0.0b4"

[jkimbo]

@patrys I just released v3.0.0b5 of graphene-django that should fix that issue. Can you try it and see?

[patrys]

@jkimbo Can confirm that it's fixed now! Now the most challenging part: to write a replacement for all promise-based dataloaders.

[patrys]

After playing with this for longer I don't see a good way to implement dataloaders.DjangoConnectionField assumes that it's completely synchronous and any code that uses functools.partial makes graphql-core assume it's synchronous as well. Now synchronous dataloaders based on Promise are no longer supported and using aiodataloader causes unresolved awaitables to be returned from resolvers (again, I think this is because graphql-core assumes that a non-coroutine wrapper cannot return an awaitable).

Edit: it seems that under Python 3.8 partials can actually function as awaitables as the code flag is set but it does not help when using DjangoConnectionField with an async connection_resolver/resolve_connection.

[jkimbo]

@patrys thanks for the investigation. We should try and use aiodataloader and use async everywhere. The lack of async DB support in Django might be a blocker though.

Could you provide some code as an example of the problems you are seeing with aiodataloader?

[patrys]

In my case dataloaders work around the ORM (at least until 3.2/4.0 lands) by offloading the database work to a worker thread:

from asgiref.sync import sync_to_async

class FooLoader(aiodataloader.DataLoader):
    @sync_to_async
    def fetch_foo(self, keys):
        return Foo.objects.in_bulk(keys)

    async def batch_load_fn(self, keys):
        foos = await self.fetch_foo(keys)
        # some other processing
        return foos

The main problem is mixing dataloaders (which require an async executor) with connection fields (that execute their own queries) within the same schema. Currently the connection field's resolve_connection/connection_resolver logic evaluates a sliced queryset which is forbidden with an event loop running (it would kill the performance of the event loop, Django will raise an exception if you try to do it) so I need a way to apply the same sync_to_async indirection to the connection field.

[cbergmiller]

graphene-django-optimizer could be an alternative to dataloader.
It has to be refactored in order to work with v3 though.

For example ist uses the following imports to analyse the query:

from graphql.execution.base import (
    get_field_def,
)
from graphql.language.ast import (
    FragmentSpread,
    InlineFragment,
    Variable,
)

These imports are all renamed or don't exist in graphql-core v3 any more.
Can someone give some pointers on what new methods from graphql-core to use?
Here is the open issue for this: https://github.com/tfoxy/graphene-django-optimizer/issues/53

[patrys]

graphene-django-optimizer works by trying to prefetch all data ahead of any resolvers running. It's by no means a replacement for working data loaders even if we assumed that the only reason to have data loaders was to wrap the ORM. The optimizer is also fully synchronous.

(Disclaimer: we've used the optimizer extensively and my team made several contributions to that project.)

[cbergmiller]

@patrys thanks for your response! Maybe i shouldn't have compared it to dataloader as my knowledge of the data loader approach is limited.
My team also uses graphene-django-optimizer. I would really like it to work with graphene-django v3. So that's why i brought it up here. Sorry if i caused any confusion.

[zbyte64]

📣
Since all the breaking changes are complete we have switched the default branch over to v3. Existing PRs are still against v2. The name of the default branch is now main and the v2 branch will continue to receive back ported fixes and releases.