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

Remove pendulum dependency #450

Merged
merged 26 commits into from
Jan 17, 2025
Merged

Conversation

svrooij
Copy link
Contributor

@svrooij svrooij commented Jan 14, 2025

Fixes #448

Remove the pendulum dependency and replace it with equivalent BCL/abstractions methods.

  • Add parseTimeDeltaFromIsoFormat function:

    • Implement a static function parseTimeDeltaFromIsoFormat in packages/abstractions/kiota_abstractions/utils.py to parse ISO 8601 duration strings.
    • Add import for re module.
  • Replace pendulum calls:

    • Replace pendulum.parse calls with equivalent BCL/abstractions methods in packages/serialization/form/kiota_serialization_form/form_parse_node.py.
    • Replace pendulum.parse calls with equivalent BCL/abstractions methods in packages/serialization/json/kiota_serialization_json/json_parse_node.py.
  • Remove pendulum dependency:

    • Remove pendulum dependency from packages/serialization/form/pyproject.toml.
    • Remove pendulum dependency from packages/serialization/json/pyproject.toml.

For more details, open the Copilot Workspace session.

Fixes microsoft#448

Remove the `pendulum` dependency and replace it with equivalent BCL/abstractions methods.

* **Add `parseTimeDeltaFromIsoFormat` function**:
  - Implement a static function `parseTimeDeltaFromIsoFormat` in `packages/abstractions/kiota_abstractions/utils.py` to parse ISO 8601 duration strings.
  - Add import for `re` module.

* **Replace `pendulum` calls**:
  - Replace `pendulum.parse` calls with equivalent BCL/abstractions methods in `packages/serialization/form/kiota_serialization_form/form_parse_node.py`.
  - Replace `pendulum.parse` calls with equivalent BCL/abstractions methods in `packages/serialization/json/kiota_serialization_json/json_parse_node.py`.

* **Remove `pendulum` dependency**:
  - Remove `pendulum` dependency from `packages/serialization/form/pyproject.toml`.
  - Remove `pendulum` dependency from `packages/serialization/json/pyproject.toml`.

---

For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/microsoft/kiota-python/issues/448?shareId=XXXX-XXXX-XXXX-XXXX).
@svrooij svrooij requested a review from a team as a code owner January 14, 2025 16:15
@svrooij
Copy link
Contributor Author

svrooij commented Jan 14, 2025

To be totally clear, I'm not a python developer this is all copilot 😄

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

great first draft!
I think a couple of things are missing besides my comments:

  • text package
  • unit tests updates

packages/abstractions/kiota_abstractions/utils.py Outdated Show resolved Hide resolved
packages/abstractions/kiota_abstractions/utils.py Outdated Show resolved Hide resolved
packages/abstractions/kiota_abstractions/utils.py Outdated Show resolved Hide resolved

This comment was marked as outdated.

Copy link
Contributor

Conflicts have been resolved. A maintainer will take a look shortly.

@svrooij
Copy link
Contributor Author

svrooij commented Jan 15, 2025

@baywet is there a command to apply the wanted code formatting? I see it's failing at that.

@baywet
Copy link
Member

baywet commented Jan 15, 2025

@svrooij should be something like yapf -ir .

@baywet
Copy link
Member

baywet commented Jan 15, 2025

@svrooij a few files were forgotten about:

  • packages\serialization\form\tests\unit\test_form_serialization_writer.py
  • packages\serialization\json\kiota_serialization_json\json_serialization_writer.py
  • packages\serialization\json\tests\unit\test_json_parse_node.py
  • packages/serialization/multipart/pyproject.toml
  • packages\serialization\json\tests\unit\test_json_serialization_writer.py

@baywet
Copy link
Member

baywet commented Jan 15, 2025

@svrooij there are additional CI issues, would you mind having a look at it please?

svrooij and others added 7 commits January 15, 2025 15:50
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
@svrooij
Copy link
Contributor Author

svrooij commented Jan 15, 2025

This is related to #453 being able to run the tests locally would greatly reduce the developer feedback flow.

Signed-off-by: Vincent Biret <vibiret@microsoft.com>
Signed-off-by: Vincent Biret <vibiret@microsoft.com>
baywet
baywet previously approved these changes Jan 16, 2025
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

Thank you for making the changes!

@baywet
Copy link
Member

baywet commented Jan 16, 2025

@svrooij there are still tests failing for form and json, would you mind having a look whenever you have some time please?

@danieleds
Copy link

danieleds commented Jan 16, 2025

The tests are failing because [datetime.]time.fromisoformat accepted a limited set of formats in Python 3.9 and 3.10.

In particular, the tests specify

    "startWorkTime=08:00:00.0000000&"
    "endWorkTime=17:00:00.0000000&"

but on older python, the fractional second part is only allowed to have either 3 or 6 digits, and we specify 7.

An option would be, if we are on an older python and there is a fractional part ("."), then we cut it to 6 if the input string is longer than 6, or pad it with zeros to 6 if it is shorter

@baywet
Copy link
Member

baywet commented Jan 16, 2025

thanks for chiming in @danieleds !!

to achieve this, would you add a condition like this one in the code?

@danieleds
Copy link

@baywet would love to, but I don't think I have push permissions on this branch

@baywet
Copy link
Member

baywet commented Jan 16, 2025

To be clear, I was only asking about how you'd do it, so @svrooij has all the elements.
We only have very few changes left for this to be complete IMHO, I don't think having multiple people working on it is going to be super productive, especially when we consider the overhead of either setting permissions or sending PRs to a fork.
Of course, if @svrooij needs more help than this guidance, we might re-evaluate at a later point.
Does that make sense?

@danieleds
Copy link

Oh I see. Yes, that's exactly what I had in mind. Something like, in form_parse_node.py and similar files:

if sys.version_info[:3] <= (3,10):
    fixed_time = re.sub(r'(\d[.,])(\d+)', lambda x: x.group(1) + x.group(2).ljust(6,'0')[:6], self._node)
    return time.fromisoformat(fixed_time)
else:
    return time.fromisoformat(self._node)

(untested)

@svrooij
Copy link
Contributor Author

svrooij commented Jan 17, 2025

Oh I see. Yes, that's exactly what I had in mind. Something like, in form_parse_node.py and similar files:

if sys.version_info[:3] <= (3,10):
    fixed_time = re.sub(r'(\d[.,])(\d+)', lambda x: x.group(1) + x.group(2).ljust(6,'0')[:6], self._node)
    return time.fromisoformat(fixed_time)
else:
    return time.fromisoformat(self._node)

(untested)

Thanks @danieleds for your input.
Since that code is used multiple times, I added it to the date_utls file.

8aaeff1#diff-8acdec4a0f6630bb416ac06117c85639c6656c1f65ef249c859dad74bc82c0ccR82-R98

@baywet
Copy link
Member

baywet commented Jan 17, 2025

@svrooij I did have a question: when I approve the CI and it fails, do you receive notifications on your end or not? I don't want to double down in case you're already receiving those :)

@svrooij
Copy link
Contributor Author

svrooij commented Jan 17, 2025

@baywet I only get notifications from reactions (yours and sonarqube)

@baywet
Copy link
Member

baywet commented Jan 17, 2025

@svrooij Thanks for confirming. That's what I suspected since I get the notifications when the CI fails. So I'll keep notifying you here if that works for you.

And yes, the CI failed again :)

@svrooij
Copy link
Contributor Author

svrooij commented Jan 17, 2025

Going to download a python 3.9 dev container and run the tests there.

@danieleds
Copy link

@svrooij looks like in the regex we also need to replace the "," with "." for older python.

Also I realized my python version check was slightly wrong, I suggest using the following instead: if sys.version_info[:3] < (3,11): (so that all patch versions of 3.10 do match too)

@danieleds
Copy link

something like this

fixed_time = re.sub(r'(\d)([.,])(\d+)', lambda x: x.group(1) + "." + x.group(3).ljust(6,'0')[:6], self._node)

@svrooij
Copy link
Contributor Author

svrooij commented Jan 17, 2025

@baywet It seems I do get the notification about failures inside github but not in my mail. Probably disabled that a long time ago.

@baywet
Copy link
Member

baywet commented Jan 17, 2025

Still seeing issues with unit tests. Could it be that Z is not supported for date time parsing in older versions?
Also might need some additional work for forms tests on times

@baywet
Copy link
Member

baywet commented Jan 17, 2025

@baywet It seems I do get the notification about failures inside github but not in my mail. Probably disabled that a long time ago.

ah ok! I'll stop posting here then to avoid spamming you

@svrooij
Copy link
Contributor Author

svrooij commented Jan 17, 2025

@danieleds and strip of the Z?

FAILED tests/unit/test_json_parse_node.py::test_get_collection_of_object_values - ValueError: Invalid isoformat string: '2021-07-29T03:07:25Z'

@danieleds
Copy link

@svrooij yes I think that's the issue. Instead of stripping the "Z", you may consider replacing it with "+00:00" so that we retain the timezone information

Copy link

@svrooij
Copy link
Contributor Author

svrooij commented Jan 17, 2025

It really helps that I'm now able to run all the tests locally in Python 3.9, only 1 error left and that is because an assertion tried to parse a datetime string with a z to compare

And sorry for spamming.

Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

CI passes!! 🚀🚀🚀🚀
Thanks for the work here @svrooij and @danieleds !

@baywet baywet merged commit 8a66dc4 into microsoft:main Jan 17, 2025
44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done ✔️
Development

Successfully merging this pull request may close these issues.

json/form/text - remove pendulum dependency
3 participants