Skip to content

remove pydantic #58

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

Merged
merged 9 commits into from
Feb 24, 2025
Merged

remove pydantic #58

merged 9 commits into from
Feb 24, 2025

Conversation

AviAvni
Copy link
Contributor

@AviAvni AviAvni commented Jan 29, 2025

Fix #55

@gkorland gkorland mentioned this pull request Jan 29, 2025
@AviAvni AviAvni changed the title update pydantic remove pydantic Jan 29, 2025
sbrudz added a commit to defmethodinc/multilspy that referenced this pull request Jan 29, 2025
@InternetOfTofu
Copy link

This PR won't work, the entire section related to LogLine needs to change when pydantic is removed:

        debug_log_line = LogLine(
            time=str(datetime.now().strftime("%Y-%m-%d %H:%M:%S")),
            level=logging.getLevelName(level),
            caller_file=caller_file,
            caller_name=caller_name,
            caller_line=caller_line,
            message=debug_message,
        )
        self.logger.log(
            level=level,
            msg=debug_log_line.json(),
        )

Without BaseModel, the class will not have a constructor. So the construction line will need to change to something like:

debug_log_line = LogLine()
debug_log_line.time  = ...
debug_log_line.level = ...
debug_log_line.caller_file = caller_file
...

This manually set all the attributes.

The .json() method from debug_log_line.json() is also from BaseModel class. This will need to change to json.dumps(vars(debug_log_line)). This will create an equivalent json to what would have been produced with BaseModel.json() method.

After this 2 changes this PR will work.

(sorry I can't submit a PR myself due to the complicated approval that I'll then need to go through 😢)

@AviAvni
Copy link
Contributor Author

AviAvni commented Feb 3, 2025

@sbrudz thanks for the review is this ready now?

@sbrudz
Copy link
Contributor

sbrudz commented Feb 4, 2025

@sbrudz thanks for the review is this ready now?

@AviAvni The fix you made looks good to me. I'm not a maintainer, though, so @LakshyAAAgrawal will need to answer your question.

@LakshyAAAgrawal
Copy link
Collaborator

@AviAvni Thank you so much for your PR!

Can you please have a look at the failed tests (https://github.com/microsoft/multilspy/actions/runs/13097207670/job/37418751364?pr=58)? Basically, "typing_extensions", which is a dependency of pydantic is still being used many places elsewhere in the repository. So we should try adding it back.

@gkorland, @wapiflapi, @camsteffen, @InternetOfTofu, @sbrudz - Thank you so much for your valuable feedback and comments!

@@ -11,7 +11,7 @@ authors = [
]
description = "A language-agnostic LSP client in Python, with a library interface. Intended to be used to build applications around language servers. Currently multilspy supports language servers for Python, Rust, Java, Go, JavaScript and C#. Originally appeared as part of Monitor-Guided Decoding (https://github.com/microsoft/monitors4codegen)"
readme = "README.md"
requires-python = ">=3.7"
requires-python = ">=3.8, <4.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you kindly undo the "<4.0" change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as I wrote if I do it poetry lock command failed with
For jedi-language-server, a possible solution would be to set the python property to ">=3.8,<4.0"

@LakshyAAAgrawal
Copy link
Collaborator

@AviAvni, I am very sorry for being so picky on this, but I want to ensure multilspy can be runnable in as many configurations and environments as possible (which is why we are removing pydantic in the first place), so I apologize for being picky, and request you to make the changes as I requested above if you think they are right!

@sternj
Copy link

sternj commented Feb 21, 2025

I also want to note that, at the moment, there's nothing except the .json() method that's distinct to Pydantic v1-- the deprecation warning indicates that it won't be removed until v3.0, so you could just expand the version range to make it work easier.

@LakshyAAAgrawal
Copy link
Collaborator

Dear @sternj, Thank you very much for bringing this to notice. Overall I think since we are already able to contain the usage of pydantic, it may be better to do away with a dependency?

@sternj
Copy link

sternj commented Feb 24, 2025

@LakshyAAAgrawal Given that it's approved now that definitely makes more sense! I was just suggesting a quick and dirty fix to satiate our dependency resolvers while the maintainers put in place a real solution! There is one now, which is far better than my idea.

@LakshyAAAgrawal LakshyAAAgrawal merged commit d1b5d03 into microsoft:main Feb 24, 2025
4 checks passed
@LakshyAAAgrawal
Copy link
Collaborator

Thank you so much @AviAvni for the contribution, and to everyone for their comments and feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uprev pydantic
7 participants