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

Removal of pendulum dependency in kiota-python breaks earlier versions of msgraph-sdk #1068

Open
chaaaaaarlie opened this issue Jan 17, 2025 · 3 comments
Labels
question Further information is requested status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close type:question An issue that's a question

Comments

@chaaaaaarlie
Copy link

Describe the bug

We are using msgraph-sdk-python 1.0.0 to automate the download and copying of files from Sharepoint/OneDrive. Until this morning, fields such as created_date_time and modified_date_time were instances of pendulum.datetime.

kiota-python's dependency on pendulum was recenly removed ( microsoft/kiota-python@8a66dc4 ), and a new release (1.8.0) of the various kiota-python artifacts was published this morning. Since msgraph-sdk-python uses imprecise versioning for microsoft-kiota-* (i.e. microsoft-kiota-serialization-json >=1.3.0,<2.0.0"), the newly published version of microsoft-kiota-serialization-json is included with all previous releases of msgraph-sdk-python and our code which relies on created_date_time and modified_date_time being instances of pendulum.datetime breaks despite not updating to a new version of the library.

Expected behavior

Release versions of a publicly available library should use exact versioning for dependencies whenever possible so that changes in the library's dependencies do not break existing versions/releases of the library.

How to reproduce

  1. Install version 1.0.0 of msgraph-sdk-python
  2. Observe that instances of created_date_time and other datetime fields are instances of datetime.datetime.

SDK Version

1.0.0

Latest version known to work for scenario above?

No response

Known Workarounds

No response

Debug output

Click to expand log ```
</details>


### Configuration

_No response_

### Other information

_No response_
@chaaaaaarlie chaaaaaarlie added status:waiting-for-triage An issue that is yet to be reviewed or assigned type:bug A broken experience labels Jan 17, 2025
@baywet
Copy link
Member

baywet commented Jan 20, 2025

Hi @chaaaaaarlie
Thank you for using the SDK and for reaching out.

The type used by the model always was datetime (non pendulum). While I understand that change is a surprise and unpleasant, there was no guarantee being made about the implementation type.

If you need functionality from pendulum, you should be able to perform a conversion with the following snippet.

dt = datetime(2008, 1, 1)
p = pendulum.instance(dt)

(source)

Let us know if you have any additional comments or questions.

@baywet baywet added question Further information is requested status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close type:question An issue that's a question and removed status:waiting-for-triage An issue that is yet to be reviewed or assigned type:bug A broken experience labels Jan 20, 2025
@chaaaaaarlie
Copy link
Author

chaaaaaarlie commented Jan 22, 2025

Hi @baywet

Thanks for the reply. I'm pretty sure you're mistaken - until the date I created my issue, these objects were delivered to the user with Pendulum datetimes. See below REPL output, where I'm explicitly specifying the previously available version of microsoft-kiota-serialization-json:

$ pip install msgraph-sdk==1.0.0 microsoft-kiota-serialization-json==1.7.1
...
$ python -m asyncio                                                       
asyncio REPL 3.10.14 | packaged by conda-forge | (main, Mar 20 2024, 12:51:49) [Clang 16.0.6 ] on darwin
Use "await" directly instead of "asyncio.run()".
Type "help", "copyright", "credits" or "license" for more information.
>>> import asyncio
>>> from azure.identity import ClientSecretCredential
>>> from msgraph import GraphServiceClient
>>> 
>>> token = 'xxx'
>>> drive_id = 'xxx'
>>> tid = 'xxx'
>>> aid = 'xxx'
>>> secret = 'xxx'
>>> 
>>> cli = GraphServiceClient(
...     ClientSecretCredential(
...         tenant_id=tid,
...         client_id=aid,
...         client_secret=secret,
...     )
... )
>>> 
>>> req = cli.drives.by_drive_id(drive_id).items.by_drive_item_id("root")
>>> response = await (req.delta_with_token(token).get())
>>> dt = response.value[0].file_system_info.created_date_time
>>> print(dt)
2023-11-13 15:23:52+00:00
>>> type(dt)
<class 'pendulum.datetime.DateTime'>
>>>

Same code without specifying the earlier version of microsoft-kiota-serialization-json:

$ pip install --no-cache-dir msgraph-sdk==1.0.0
...
Successfully installed microsoft-kiota-serialization-json-1.9.0 msgraph-sdk-1.0.0
...
$ python -m asyncio                            
...
>>> dt = response.value[0].file_system_info.created_date_time
>>> print(dt)
2023-11-13 15:23:52+00:00
>>> type(dt)
<class 'datetime.datetime'>
>>> 

More to the point, I'm not trying to restore this state - the only reason I have references in my code to these Pendulum objects is so that I can convert them into vanilla datetime.datetime objects.

there was no guarantee being made about the implementation type

This is exactly the problem I'm trying to point out - for a public-facing API library, not guaranteeing the data types you'll receive in an API response, and having those types subsequently change due to an underlying dependency updating, is extremely undesirable behavior. If I use msgraph-sdk 1.0.0 on Tuesday and I get back a different response for the same parameters I used on Monday, it's not a good look. I don't see any reason why a versioned release of this library would not exactly specify the version of all dependencies in use.

Please let me know your thoughts. I think the release strategy for this library needs to be rethought to ensure stability.

Thanks,

Charlie

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 and removed status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels Jan 22, 2025
@baywet
Copy link
Member

baywet commented Jan 22, 2025

Thank you for the additional information.

While I understand this change can be destabilizing I don't think our team is going to make that additional guarantee in the future unless we get overwhelming feedback from customers it's needed. Please understand that while I'm going to provide more details below, I do acknowledge those kinds of changes are harder to catch. And we should probably at least have done a better job at outlining the change in the changelog.

It'd make the SDK too rigid and wouldn't allow us to swap encapsulated implementation types as needed.

The API surface type guarantee to prevent source breakages is BCL datetime. And that was maintained through the change.

Since pendulum datetime inherits from BCL datetime all that was "lost" was the type in the hierarchy as well as additional methods and behaviours from that type.

But again, the API surface never guaranteed it'd return that type to begin with.

If applications have a strong dependency on that type, they should have tested/converted returned values from the SDK API surface, added a direct dependency etc...

Let me know if you have additional questions or concerns here.

@baywet baywet added status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close and removed Needs: Attention 👋 labels Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close type:question An issue that's a question
Projects
None yet
Development

No branches or pull requests

2 participants