Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new context argument to _get_responses method #118

Merged

Conversation

anubhavsharma515
Copy link
Contributor

@anubhavsharma515 anubhavsharma515 commented Dec 27, 2022

Fixes #116 (and #113 I believe).

From the linked issue, a recent modification to the json-rpc parent JSONRPCResponseManager https://github.com/pavlov99/json-rpc/pull/122/files#diff-71ff9d558b2e51825f517c97faa2f119ab0aaf26aaff43e36731719017cce06eR75 class adds a new argument that is causing an error in the overridden version in the custom dbt-rpc ResponseManager class.

This PR essentially just adds in the same context argument to the _get_responses method, which is being called from the parent handle_request method.

@ChenyuLInx
Copy link
Contributor

ChenyuLInx commented Jan 4, 2023

@anubhavsharma515 Thanks for the fix!! I don't think this would resolve #113 but def resolves #116. I think internally we got over the issue by pinning a previous version of json-rpc. But this should to be the way longer term.

@colin-rogers-dbt
Copy link
Contributor

This makes sense, need to debug the failing unit tests

Copy link
Contributor

@colin-rogers-dbt colin-rogers-dbt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anubhavsharma515 in order for this to work you would need to update the requirements.txt to specify the new version of json-rpc as my change pinned it to the previouse version. See: 43a1399

See unit test failures for more but:

  File "/home/runner/work/dbt-rpc/dbt-rpc/dbt_rpc/rpc/response_manager.py", line 92, in _get_responses
    for output in super()._get_responses(requests, dispatcher, context):
TypeError: _get_responses() takes 3 positional arguments but 4 were given

@anubhavsharma515
Copy link
Contributor Author

Hey @ChenyuLInx and @colin-rogers-dbt, many thanks for the review and also for pointing out the issue with the CI tests.
I reset the setup.py to what it was before the version was pinned and hopefully this is the right way to go.

Kindly let me know if there's any further changes required on my end (and thank you so much for taking the time to review again).

setup.py Outdated
@@ -29,7 +29,7 @@ def read(fname):
],
},
install_requires=[
'json-rpc~=1.13.0',
'json-rpc>=1.12,<2',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be >=1.14? since the _get_responses doesn't take that context argument before 1.14?

Copy link
Contributor Author

@anubhavsharma515 anubhavsharma515 Jan 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh you're right! But just confirming the tags in the json-rpc repo, would it be >=1.13?
Nvm! Apologies for the confusion, I shall push the change!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick adjust of it!!!!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely! Thanks so much for such a thorough review :)

@ChenyuLInx
Copy link
Contributor

@anubhavsharma515 Thank you so much for looking into this and update!! Just one last question and I am happy to merge after it is being confirmed!

@ChenyuLInx ChenyuLInx merged commit 521ec41 into dbt-labs:main Jan 5, 2023
@anubhavsharma515 anubhavsharma515 deleted the fix/rpc-response-manager-method branch January 5, 2023 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-1730] RPC Failure after recent update to source JSON-RPC JSONRPCResponseManager class
3 participants