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

Switch to python 3.8 #1389

Closed
15 tasks done
rwols opened this issue Oct 22, 2020 · 34 comments
Closed
15 tasks done

Switch to python 3.8 #1389

rwols opened this issue Oct 22, 2020 · 34 comments
Milestone

Comments

@rwols
Copy link
Member

rwols commented Oct 22, 2020

Before we can use asyncio in all of its glory, we need to switch to the Python 3.8 runtime. To do that, we have to wait for a few things:

  • Package Control must implement an upgrade path for existing "dependencies"
  • The backrefs dependency must be upgraded to 3.8
  • The dependency bracex must be upgraded to 3.8
  • The dependency markupsafe must be upgraded to 3.8
  • The dependency mdpopups must be upgraded to 3.8
  • The dependency pygments must be upgraded to 3.8
  • The dependency pymdownx must be upgraded to 3.8
  • The dependency python-jinja2 must be upgraded to 3.8
  • The dependency python-markdown must be upgraded to 3.8
  • The dependency pyyaml must be upgraded to 3.8
  • The dependency wcmatch must be upgraded to 3.8
  • sublime_lib must be upgraded to 3.8
  • lsp_utils must be upgraded to 3.8
  • have a pull request ready for switching to 3.8 for the LSP package
  • announce deadline for switching all helper packages

As far as I understand, this is also going to be a hard backwards-incompatible break. So we have to announce it.

@rwols rwols added this to the 2.0.0 milestone Nov 5, 2020
@rwols
Copy link
Member Author

rwols commented Jan 22, 2021

Package Control WIP branch is at https://github.com/wbond/package_control/tree/four-point-oh

@rchl
Copy link
Member

rchl commented Jan 24, 2021

Realistically also lsp_utils and sublime_lib would have to be updated or most of the LSP-* packages won't work.

I only really use ResourcePath from sublime_lib so I could vendor it but it will probably be updated anyway since it's from an active ST member.

@predragnikolic
Copy link
Member

I think the time for Python 3.8 if finally here :)

Just a small note,
the list of deps that LSP currently uses is this:

            "bracex",
            "mdpopups",
            "pathlib",
            "wcmatch",

I did a quick experiment.

I have only LSP and LSP-pyright setup.
I created a .python-version file at the root of LSP and LSP-pyright.
The file just contains 3.8.

Once I did that, I expected that I need to change something in lsp_utils,
but I didn't. I restarted ST, both LSP and LSP-pyright were running in 3.8, which is great.

I don't know why I didn't need to change anything in lsp_utils...
it looked like PC4 already added lsp_utils to Lib/python38 with the rest of the required LSP dependencies:

~/.config/sublime-text/Lib/python38  ls
bracex                    lsp_utils-3.1.0.dist-info  sublime_lib-1.5.2.dist-info
bracex-2.1.1.dist-info    mdpopups                   wcmatch
coverage                  mdpopups-4.2.2.dist-info   wcmatch-1.2.1.dist-info
coverage-7.4.1.dist-info  package_control.py
lsp_utils                 sublime_lib

@jwortmann
Copy link
Member

jwortmann commented Jan 27, 2024

I don't really know how Package Control works, but I can see two unmerged pull requests wbond/package_control_channel#8713 and wbond/packagecontrol.io#157 for the channel and for the website, whose descriptions indicate that they are required to install dependencies/libraries on the 3.8 environment:

2. opt-in compatible libraries to python 3.8

Why channel_v4.json

Package Control 4.0 handles both 3.0.0 and 4.0.0 schemes well. Just requires 4.0.0 to pull dependencies/libraries for python 3.8.

Notably, the pathlib dependency seems to be still restricted to 3.3 in the package_control_channel PR (note that it's also not listed in your ls output):

pathlib

So, as far as I understand, the linked PR needs to be merged first, and even after that we can't upgrade to Python 3.8 unless we drop the pathlib dependency. Is that correct?

Edit: Ah, pathlib is of course part of the Python 3.8 stdlib, so we can just remove it from the dependencies and import from the stdlib directly in the code. I guess then it makes sense why the dependency is only for Python 3.3 😄


Also as far as I understand, all LSP-* helper packages will need a .python-version file with 3.8 as well to keep working. So we need to update them all at the same time with a new LSP 2.0.0 (Python 3.8) release. I think this occasion could potentially be used in case we want to do some other breaking API changes. If we want to do something like that, maybe we should start another thread/issue to collect and discuss such possible API changes, before pushing for a 3.8 update. Let me start here with a first example:

  • Remove the SessionViewProtocol and SessionBufferProtocol classes - as far as I can tell, they don't have any purpose, and in my opinion they make working with parts of the code base very annoying because "Goto definition" for methods from the SessionView and SessionBuffer classes doesn't work (it only opens the empty method stubs from the Protocol classes). Also I have never seen something like this where an interface/protocol is introduced for something that will ever only have a single class which uses the interface. So I'd vote to remove those two Protocol classes.

@rchl
Copy link
Member

rchl commented Jan 27, 2024

  • Remove the SessionViewProtocol and SessionBufferProtocol classes - as far as I can tell, they don't have any purpose, and in my opinion they make working with parts of the code base very annoying because "Goto definition" for methods from the SessionView and SessionBuffer classes doesn't work (it only opens the empty method stubs from the Protocol classes). Also I have never seen something like this where an interface/protocol is introduced for something that will ever only have a single class which uses the interface. So I'd vote to remove those two Protocol classes.

I believe the purpose is to clearly define what is a public interface and what is internal. Especially for the plugin packages accessing those. I'm not sure how well it's working in practice... I suppose there are cases already where the plugins just override the type and access things anyway.

@jwortmann
Copy link
Member

I believe the purpose is to clearly define what is a public interface and what is internal. Especially for the plugin packages accessing those. I'm not sure how well it's working in practice... I suppose there are cases already where the plugins just override the type and access things anyway.

Besides the fact that there is no way in Python to enforce this (those are only type annotations in the plugin class methods anyway) and as you wrote plugins can easily workaround it, Python already has a convention to mark private methods with an underscore. So in my opinion those protocol classes have zero advantages and only make working with the code harder if you use tools like Pyright which use the type annotations as source for "Goto Definition". And around half of the methods from the protocol classes are not meant to be used by plugins anyway and instead were just added there to satify the type checker. Furthermore, there are other classes exposed in the plugin API which apparently also work fine without having a protocol class, e.g. Session without a SessionProtocol etc.

I can also see no indication in https://peps.python.org/pep-0544/ that the intention of typing.Protocol would be to hide internal methods, so I still think this is a misuse or at least bad practice here.

@rchl
Copy link
Member

rchl commented Jan 27, 2024

I get your point. It seems a bit excessive given that we have only one implementation for each of those protocols.

That said, regardless if misuse or not, it does make things easier for plugin developers to limit the stuff that they can see (type-wise). For example SessionView has a lot of methods that the plugins should not touch (like on_before_remove, on_capability_added_async and other) but at the same time those can't be marked as private as we want to treat those as public within the LSP code base.

I do think removing those would simplify things within the LSP code base which would be nice. But do we want to accept the before-mentioned drawback? Probably the ideal way would be to have more restricted public Session interface that would then return those more restricted interfaces and those would only be used by external plugins, without affecting internal code.

@rwols
Copy link
Member Author

rwols commented Jan 28, 2024

From what I remember, the reason the two protocol classes SessionViewProtocol and SessionBufferProtocol exist is because they avoid a circular import problem. If the code in sessions.py imported one of the two concrete implementation classes then that would result in a circular import. So, the initial reason for them existing was a technical reason.

Maybe that circular import problem doesn't exist anymore, in which case it's totally fine to remove them if possible (from a technical standpoint).

@rchl
Copy link
Member

rchl commented Jan 28, 2024

This is most likely still a problem

@jwortmann
Copy link
Member

jwortmann commented Jan 28, 2024

I've checked it and it turns out that both SessionViewProtocol and SessionBufferProtocol are only used in type annotations. Therefore it should be possible to avoid circular imports by using

if TYPE_CHECKING:
    from ..session_view import SessionView
    from ..session_buffer import SessionBuffer

and make those type annotations to be strings. AFAIK the latter is not even necessary anymore on Python 3.8 if you add from __future__ import annotations to the relevant files.

@deathaxe
Copy link
Contributor

deathaxe commented Feb 10, 2024

FWIW, I am running LSP and all its helpers seamlessly on python 3.8 for years, since it was initially possible to somehow install dependencies for python 3.8 in early PC4.0 dev builds.

Package Control 4 uses https://github.com/packagecontrol/channel to organize python 3.8 dependencies until there's a chance to upgrade packagecontrol.io.

New depednecies/libraries need to be registered at https://github.com/packagecontrol/channel.

So basically any package/plugin can migrate to python 3.8

The only challenge is probably to migrate LSP and all its helpers at the same time as those need to runn all on same plugin_host.


lsp_utils is registered at https://github.com/packagecontrol/channel/blob/8142a3b67d2ee640d8769cbfcec593e6e4df5554/repository.json#L478-L497 to provide smooth transition experience.

@predragnikolic
Copy link
Member

predragnikolic commented Feb 10, 2024

So in order for LSP to update, can you help me figure out what is needed:

  1. Add a .python-version with 3.8 in it, to LSP and all LSP-* pacakges.
  2. Remove the pathlib dependecy - it is no longer needed.
  3. Make a release of LSP and LSP-* packages at the same time.
    • this is not a issue for LSP-* packages in the SublimeLSP org, but we need to figure out what to do for LSP-* packages outside of the organization.

Q1 Do I need to add/change something in lsp_utils?
Q2 Should I do something else regarding the update process?
Q3 Although this is a big change, I don't know if I should see this as a breaking change or not :) how do you see it?

@predragnikolic
Copy link
Member

predragnikolic commented Feb 10, 2024

Here is the complete lists of LSP-* packages:
Package name (maintainer) Releases(tags)
Chialisp (cameroncooper) >=4000
LSP 3154 - 4069(3154-), >=4070(4070-)
LSP-angular 3154 - 3999(st3-), >=4000
LSP-astro >=4070
LSP-bash 3154 - 3999(st3-), >=4000
LSP-Bicep >=4070
LSP-biome >=4070
LSP-clangd >=4070
LSP-copilot (TheSecEng) >=4126
LSP-cspell >=4126
LSP-css 3154 - 4147(st3-), >=4148
LSP-Dart >=4070
LSP-Deno >=4070
LSP-dockerfile 3154 - 3999(st3-), >=4000
LSP-elixir 3154 - 3999(st3-), >=4000
LSP-elm 3154 - 3999(st3-), >=4000
LSP-eslint 3154 - 3999(st3-), >=4000
LSP-file-watcher-chokidar >=4070
LSP-flow >=4070
LSP-gnols (jdkato) >=4070
LSP-gopls >=4070
LSP-Grammarly >=4070
LSP-graphql 3154 - 3999(st3-), >=4000
LSP-html 3154 - 4147(st3-), >=4148
LSP-intelephense 3154 - 3999(st3-), >=4000
LSP-jdtls >=4070
LSP-json 3154 - 4147(st3-), >=4148
LSP-julia >=4095
LSP-kotlin >=4070
LSP-lemminx 3154 - 4069(st3-), >=4070
LSP-leo >=4070
LSP-ltex-ls >=4070
LSP-lua >=4070
LSP-marksman >=4070
LSP-metals (scalameta) 3154 - 3999(st3-), >=4000
LSP-OmniSharp >=4070
LSP-PowerShellEditorServices >=4070
LSP-prisma (Sublime-Instincts) >=4126
LSP-promql (prometheus-community) 3154 - 4069
LSP-pylsp 3154 - 3999(st3-), >=4000
LSP-pyright 3154 - 4147(st3-), >=4148
LSP-R >=4070
LSP-rome >=4070
LSP-ruff 3154 - 3999(st3-), >=4000
LSP-rust-analyzer >=4070
LSP-serenata 3154 - 3999
LSP-SourceKit >=4070
LSP-stylelint 3154 - 3999(st3-), >=4000
LSP-svelte 3154 - 3999(st3-), >=4000
LSP-tagml (HuygensING) 3154 - 4069
LSP-tailwindcss 3154 - 3999(st3-), >=4000
LSP-terraform >=4070
LSP-TexLab >=4070
LSP-typescript 3154 - 3999(st3-), >=4000
LSP-vale-ls (errata-ai) >=4070
LSP-VHDL-ls (martinbarez) >=4070
LSP-volar 3154 - 4147(st3-), >=4148
LSP-vue 3154 - 3999(st3-), >=4000
LSP-yaml 3154 - 4147(st3-), >=4148
LSP-anakin
LSP-cmake
LSP-SonarLint
lsp_utils 3000 - 4069(st3-v), >=4070

The following list of LSP-* packages are outside of the org. We would need to figure out how to sync the LSP release and the release of LSP-* packages outside of the org.

Chialisp (cameroncooper),
LSP-copilot (TheSecEng),
LSP-gnols (jdkato),
LSP-metals (scalameta),
LSP-prisma (Sublime-Instincts),
LSP-vale-ls (errata-ai),
LSP-VHDL-ls (martinbarez)

The following packages will not be updated to py3.8:

  • LSP-tagml (outside from organization), LSP-serenata, LSP-promql(outside from organization) because they only support ST3.

@deathaxe
Copy link
Contributor

Q1 Do I need to add/change something in lsp_utils?

No. It's working fine as it is.

Q2 Should I do something else regarding the update process?

The 3. steps described, are technically everything, which is needed.

Q3 Although this is a big change, I don't know if I see this a breaking change or not :) how do you see it?

It isn't a breaking change functionality wise.

It is however a breaking change for LSP-* helper packages as they need to opt-in to python 3.8 as well to ensure to run on same plugin_host. This probably deserves a milestone or landmark to see which versions of LSP and LSP-* are compatible.

That's however only an organizational thing as there's no dependency and version management available accross packages.

At least a message for users would be good, to point this change out.

@rchl
Copy link
Member

rchl commented Feb 11, 2024

I don't know if a message to users would be needed if the intention is that there will be no visible changes in the behavior...

I don't think most users would really care about some internal detail regarding the runtime version the package runs on.

@rchl
Copy link
Member

rchl commented Feb 11, 2024

Note that when I've tried LSP on python 3.8 before I've noticed that there is some breaking behavior regarding string enum handling. In 3.3 runtime it's not handled any differently than without annotation while in 3.8 I think there were issues with stringifying the value or something...

@deathaxe
Copy link
Contributor

If so, I - as an end user - haven't noticed them.

@jwortmann
Copy link
Member

Q3 Although this is a big change, I don't know if I should see this as a breaking change or not :) how do you see it?

I would say this is clearly a breaking change. There could be people using a local helper plugin instead of the "clients" configuration in the settings, because some functionality is only available from the plugin class. And it might not be published to GitHub because they don't want to maintain server installation & updates, or for other reasons. So I would recommend to add a small note about it to the update message (maybe something like "LSP now uses the Python 3.8 plugin host. For plugin compatibility, developers of helper packages need to include a .python-version file into their package now."). And the version number of LSP should be incremented to 2.0.0.


There is (at least) one other package outside of the sublimelsp org that you missed above and which needs to be updated:
https://packagecontrol.io/packages/WolframLanguage
https://github.com/WolframResearch/Sublime-WolframLanguage/blob/master/plugin.py

@predragnikolic
Copy link
Member

I created a ticket to track the progress -> #2417

I would like to keep the communication in this thread to not clutter #2417 :)

@predragnikolic

This comment was marked as outdated.

@predragnikolic
Copy link
Member

I've added the proposed versions for each LSP package and it can be seen in this issue #2417

The format is the following:
- [] PACKGE_NAME CURRENT_TAG -> PROPOSED_TAG

Example:
- [] LSP-biome 1.3.2 -> 2.0.0


Others feel free to take a look and suggest changes.

@deathaxe I know that LSP-astro tries to follow the server version releases. I put 3.0.0 from the current 2.7.3, feel free to suggest a change.

@rwols
Copy link
Member Author

rwols commented Feb 15, 2024

I would suggest not picking a "deadline" until UnitTesting works with PC4 dependencies. There is no control over UnitTesting and we can't guarantee it'll even be fixed before March 9.

@predragnikolic
Copy link
Member

That 9th March means nothing if the two bullet points get in the way. (currently, one of them is getting in the way)

And I know that things will not be fix by themself,
and the reality is probably that the CI will not be fixed by that date.

I want to have an optimistic point of view
and if by any chance, the CI gets fixed and nothing unexpected happens,
I will be more than ready to proceed with releasing 3.8.

@predragnikolic
Copy link
Member

Small update,
I do not think that the 9th March will happen. :)

I saw that deathaxe proposed some fix on the LSP discord channel. I haven't took the time to understand it. (I took a look but I didn't understand what to do with that info)

So, I will wait for things to get resolved one at a time,
when the CI blocker gets fixed somehow,
I will then notify maintainers outside of the organization for the future date.

@deathaxe
Copy link
Contributor

deathaxe commented Mar 4, 2024

UnitTesting currently ships OS dependend scripts (Bash/Powershell) to roughly perform the following

  1. Sublime Text
  2. Download Package Control
  3. Download/Clone Unittesting
  4. start ST
  5. run "satisfy dependencies" to install coverage
  6. exit ST

then ...

  1. clone package to test
  2. start ST
  3. run Unittest command

... all running on a custom docker image.

What my proposal or better proof of concept does is to perform everything after downloading ST from within the running ST instance using an installer_plugin - which actually already exists to perform step 5.

This way setup and test action could hopefully be merged and ST wouldn't need to be restarted, which might even reduce runtime.

@deathaxe
Copy link
Contributor

deathaxe commented Mar 4, 2024

P.S.: I wouldn't count on things resolving themself.

@predragnikolic
Copy link
Member

predragnikolic commented Mar 4, 2024

P.S.: I wouldn't count on things resolving themself.

I know.. I know 🙂 I just hesitate to open another Pandora box, where I need to invest time to learn.

And thanks for the above explanation.

@LDAP
Copy link
Contributor

LDAP commented Mar 28, 2024

When switching to 38 we have to also inform @daveleroy to update the LSP bridge in Debugger.

@predragnikolic
Copy link
Member

predragnikolic commented Mar 28, 2024

I have some news! 🎉

LSP and LSP-* packages by SublimeLSP are moving to Python 3.8 on April 19, 2024.

Important Date

Fri Apr 19 2024 17:00:00 GMT+0000

The Plan

  1. All LSP PRs under SublimeLSP will be merged.
  2. And a release will be created for LSP and LSP-* packages under SublimeLSP.

For maintainers of LSP-* packages outside SublimeLSP:

  1. Wait for the LSP release. (I will write an update in this thread once LSP is released)
  2. Merge the PR I've created.
  3. Create a release with a tag.
  4. You're done.

To assist maintainers outside the organization, I've already made the PRs:

Please react with 👀 when you see this message :)
cc @cameroncooper, @jfcherng, @TerminalFi, @jdkato, @ayoub-benali, @Ultra-Instinct-05, @martinbarez, @bostick

Other things to know

  • If any major issues arise before April 19, the release date will be canceled and I'll update this thread accordingly.
  • LSP-* packages not transitioning to Python 3.8 will stop working.
  • Sublime Text 3 LSP and LSP-* packages are not affected by this change.

If someone has any questions/concerns feel free to ask.

@predragnikolic
Copy link
Member

It makes sense that LSP gets a major version update,
But not so much for LSP-* packages.

I planned to do a major version update for LSP-* packages,
but after talking to rchl decided to do a minor version update.

I'll update this ticket accordingly soon.

@predragnikolic
Copy link
Member

Hello 👋

in ~one hour the LSP release will be created :)

@predragnikolic
Copy link
Member

The LSP v2 release is created -> https://github.com/sublimelsp/LSP/releases/tag/4070-2.0.0

Maintainers who maintain LSP-* packages outside of the SublimeLSP organization.
feel free to merge the PR I've created and create a new release.

@predragnikolic
Copy link
Member

I think that this can also be closed now :)

@daveleroy
Copy link

When switching to 38 we have to also inform @daveleroy to update the LSP bridge in Debugger.

This is complete

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

No branches or pull requests

7 participants