-
Notifications
You must be signed in to change notification settings - Fork 169
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
Migrate to pydantic >= 2.0 #613
Conversation
This PR is as far as I can reasonably take it. I fixed the linting errors introduced by my changes (but not the ones already existing in the master branch), and I reverted Pydantic to 2.5.3 (to make it compatible with CI environment). That said, Hivemind is also compatible with 2.7.3. I tested this change with a multi-node training run, and it works well. I also tested this with Petals, and was able to run inference there as well. I'd say this is ready for merge. |
It looks like #612 might fix the errors we're seeing in tests here. |
NB: We'll need to update hivemind to fix incompatibility with the latest torch update to so we can test this (see failed tests). I intend to do this as soon as I have some bandwidth, but if someone's willing to do this earlier, please do. |
So, it appears that Strangely, some of these tests will install torch 2.3.1, while others are installing 1.9.0 (and thus, failing). Not sure if that is intentional, but I'll keep digging. |
yes it should fix all ci error, expect the black/isort one |
fy: #612 |
It looks like mine and @samsja's PR are addressing the same bug, so we should only merge one. That said, I have no idea why these tests are failing right now, while theirs worked. All I'm seeing is Both PRs still have the same bug, relating to the Albert test:
Apparently, the "imp" module was removed in Python 3.12, so I'm sure that's where the problem lies. I was able to reproduce this bug in Docker locally, so I'll spend some time offline troubleshooting. |
It seems that If anyone with Pydantic experience wants to jump in, I'd appreciate it. |
I just tried your PR with pydantic 2.5.3 and 2.7.3. Seems to be working fine, at least the test is not hanging for me. Happy to help if you hint me on how to reproduce the hanging behavior |
The code is not hanging anymore, but we are getting some failures relating to the validator. From what I can tell, Easiest way to reproduce is with the docker compose build
docker compose up That will execute the tests specified in |
I reverted to v1 API, since v2 would probably break Pydantic usage in Petals as well... and I don't want to deal with that. It seems that most tests are passing now. I have no idea why some of them are being cancelled, but it seems like they're timing-out. And these failures are not even consistent; whereas 3.8 was successful on the previous run, it timed-out on this attempt. Not sure what's happening there, but it looks like a Github issue more than anything. |
These tests keep failing with random, transient errors. What works on this run, will fail on the next. Hivemind is just being unreliable right now. |
I'd recommend merging, if you're okay with this code. Tests keep failing, but they are failing with transient errors that were probably not introduced by the Pydantic update:
Stuff like this is just a quirk of Hivemind. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! However, before merging, please reduce the diff of the PR to make sure all changes are Pydantic-related
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you again for the pull request! I believe we are close to merging, but there is still a small list of changes that need to be made before this
requirements.txt
Outdated
@@ -12,5 +12,5 @@ configargparse>=1.2.3 | |||
py-multihash>=0.2.3 | |||
multiaddr @ git+https://github.com/multiformats/py-multiaddr.git@e01dbd38f2c0464c0f78b556691d655265018cce | |||
cryptography>=3.4.6 | |||
pydantic>=1.8.1,<2.0 | |||
pydantic>=2.5.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This version is quite recent, can you provide any reasoning behind how it was chosen? Maybe we can bump to just 2.0? Ideally, we should even keep backwards compatibility with older versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I chose this version because it was the highest possible version I could use, which was still compatible with all of the tests. Remember: the whole reason we're upgrading Pydantic is because this old version has been conflicting with other dependencies, in other projects.
We can revert to 2.0.0 if you still want me to do that, though 2.5.3 seems to be working fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, If the code is still compatible with pydantic v1 it might be worth it to just do pydantic>1.8.1
@mryab - is there anything else you need from me? Between the 3 open PRs, we should be able to get past these failing tests. |
I have been using this PR for couple of weeks, seems to be stable. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #613 +/- ##
==========================================
+ Coverage 85.39% 85.87% +0.47%
==========================================
Files 81 81
Lines 8006 8014 +8
==========================================
+ Hits 6837 6882 +45
+ Misses 1169 1132 -37
|
* Revert to v1 api * Use 2.0.0 as the minimum version (cherry picked from commit 128ee90)
This is a first pass at upgrading Pydantic to 2.7.3, as discussed in #597. Fortunately, Pydantic is barely used in Hivemind - so this should be fairly straightforward.
So far, I have extended the
BaseModel
class, in order to define aConfig
object witharbitrary_types_allowed = True
. From what I can tell, this was the simplest way to deal with our validator's regex handling. The__get_validators__
method was removed in 2.0, and this function needed some work.I also removed the
_patch_schema
calls, because 1)field.required
was changed tofield.is_required
in 2.0, and 2)field.is_required
is now read-only. I couldn't figure out how to set it... and the code seems to work with out it? So is it really necessary here?DO NOT MERGE; this is an initial attempt. Within the next few days, I will test this in a multi-node environment, and I will test it with Petals as well.
Until then, this version of the code seems to work well. I wanted to open the PR, to get us all started.