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 upperbound to microsoft-kiota-abstractions #43021

Merged
merged 4 commits into from
Oct 15, 2024

Conversation

rawwar
Copy link
Collaborator

@rawwar rawwar commented Oct 15, 2024

Currently, MyPy check is failing as we don't have a pin on microsoft-kiota-abstractions.

roviders/src/airflow/providers/microsoft/azure/hooks/msgraph.py:51: error:
Module "kiota_abstractions.request_information" has no attribute "QueryParams";
maybe "QueryParameters"?  [attr-defined]
        from kiota_abstractions.request_information import QueryParams
        ^
providers/src/airflow/providers/microsoft/azure/hooks/msgraph.py:65: error:
"NativeResponseType" has no attribute "json"  [attr-defined]
                return response.json()
                       ^~~~~~~~~~~~~
providers/src/airflow/providers/microsoft/azure/hooks/msgraph.py:66: error:
"NativeResponseType" has no attribute "content"  [attr-defined]
            content = response.content
                      ^~~~~~~~~~~~~~~~
providers/src/airflow/providers/microsoft/azure/hooks/msgraph.py:68: error:
"NativeResponseType" has no attribute "headers"  [attr-defined]
                return {key: value for key, value in response.headers.item...
                                                     ^~~~~~~~~~~~~~~~
providers/src/airflow/providers/microsoft/azure/hooks/msgraph.py:71: error:
Signature of "handle_response_async" incompatible with supertype
"ResponseHandler"  [override]
        async def handle_response_async(
        ^

They recently released newer minor version which has a breaking change.

Breaking change.
following module missing: 1.4.0 . vs 1.3.3

They released newer version at Mon, 14 Oct 2024 17:09:01 GMT . Source: https://pypi.org/rss/project/microsoft-kiota-abstractions/releases.xml

@eladkal
Copy link
Contributor

eladkal commented Oct 15, 2024

They recently released newer version which has a breaking change

Is there a reason we can't just fix the problem and exclude the problematic release? We limit version only if the fix isn't easy and requires some effort

@rawwar
Copy link
Collaborator Author

rawwar commented Oct 15, 2024

They recently released newer version which has a breaking change

Is there a reason we can't just fix the problem and exclude the problematic release? We limit version only if the fix isn't easy and requires some effort

They have entirely removed the module. So, unless we update the provider code, just excluding the specific version won't help. I guess, we can remove the constraint once we fix the provider by removing references to the removed classes/methods

@eladkal
Copy link
Contributor

eladkal commented Oct 15, 2024

They have entirely removed the module. So, unless we update the provider code, just excluding the specific version won't help. I guess, we can remove the constraint once we fix the provider by removing references to the removed classes/methods

If the fix is easy then lets fix :)

@rawwar
Copy link
Collaborator Author

rawwar commented Oct 15, 2024

If the fix is easy then lets fix :)

I can definitely take this work, but I don't think it's straightforward. At least not to me, as it's the first time I will be working with this provider. It failed the MyPy checks on the other PR I'm working on. Would it be fine if I created an issue and assigned myself?

@eladkal eladkal requested a review from potiuk October 15, 2024 12:04
@eladkal
Copy link
Contributor

eladkal commented Oct 15, 2024

I can definitely take this work, but I don't think it's straightforward. At least not to me, as it's the first time I will be working with this provider. It failed the MyPy checks on the other PR I'm working on. Would it be fine if I created an issue and assigned myself?

yes but please also add comment to the provider yaml and add the link to the issue so we have better tracking of it

@potiuk
Copy link
Member

potiuk commented Oct 15, 2024

Yes. Crating an issue and referring to it in the comment just above the dependency (as we do for other cases like that) is the way we do it. This makes it later very easy to check "has this issue been followed? What is the status? Should we do something with it"? This is all very difficult if we do not know what's the reason of upper-binding a dependency, so our rule-of-thumb is that we never upper-bind any dependency without a comment explaining why, what is the condition of removing the binding and linking to appropriate issue.

@potiuk
Copy link
Member

potiuk commented Oct 15, 2024

Also once you create the issue - we could ask other people who are involved in microsoft provider - maybe @ambika-garg could help with it for example ?

@ambika-garg
Copy link
Contributor

Hey @potiuk ,I'd be happy to assist with this issue.

@rawwar
Copy link
Collaborator Author

rawwar commented Oct 15, 2024

Yes. Crating an issue and referring to it in the comment just above the dependency (as we do for other cases like that) is the way we do it.

Thanks. I've updated the PR.

@ambika-garg, I just created #43036

@rawwar rawwar requested a review from eladkal October 15, 2024 13:48
@potiuk potiuk merged commit 7b151d3 into apache:main Oct 15, 2024
103 checks passed
@rawwar rawwar deleted the kalyan/cicd/azure_kusto_data_upperbound branch October 16, 2024 14:24
R7L208 pushed a commit to R7L208/airflow that referenced this pull request Oct 17, 2024
* add upperbound to microsoft-kiota-abstractions

* add

* add comment about breaking change
jscheffl added a commit to jscheffl/airflow that referenced this pull request Oct 19, 2024
jscheffl added a commit that referenced this pull request Oct 20, 2024
* Update trove-classifiers in v2-10 test

* Back-Port Add upperbound to microsoft-kiota-abstractions (#43021)
potiuk pushed a commit to potiuk/airflow that referenced this pull request Oct 22, 2024
* add upperbound to microsoft-kiota-abstractions

* add

* add comment about breaking change

(cherry picked from commit 7b151d3)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants