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

Add Codeium #8780

Merged
merged 3 commits into from
Jul 9, 2023
Merged

Add Codeium #8780

merged 3 commits into from
Jul 9, 2023

Conversation

fortenforge
Copy link
Contributor

@fortenforge fortenforge commented Jun 17, 2023

  • I'm the package's author and/or maintainer.
  • I have have read the docs.
  • I have tagged a release with a semver version number.
  • My package repo has a description and a README describing what it's for and how to use it.
  • My package doesn't add context menu entries. *
  • My package doesn't add key bindings. **
  • Any commands are available via the command palette.
  • Preferences and keybindings (if any) are listed in the menu and the command palette, and open in split view.

My package is Codeium, an AI-powered code completion tool.

There are no packages like it in Package Control.

@braver
Copy link
Collaborator

braver commented Jun 21, 2023

Please add your package to the "c" repository file.

@fortenforge
Copy link
Contributor Author

Are you sure I need to add it to the "c" file? I'm following the instructions here with the "Manual Hosting" route and it doesn't mention that: https://packagecontrol.io/docs/submitting_a_package#Step_6

@braver
Copy link
Collaborator

braver commented Jun 22, 2023

Oh, sorry, you're proposing a "closed source" package. Any particular reason for that? You don't seem to be monetising it directly. Note that sublime-package files are simply zips that can be extracted.

@jfcherng
Copy link
Contributor

jfcherng commented Jun 22, 2023

@jfcherng
Copy link
Contributor

jfcherng commented Jun 22, 2023

Not sure whether this will work for package updating. Especially, putting a newer version after a older one.

{
    "schema_version": "3.0.0",
    "packages": [
        {
            "name": "Codeium",
            "releases": [
                {
                    "sublime_text": "*",
                    "version": "1.2.26",
                    "url": "https://github.com/Exafunction/codeium-sublime/raw/main/Codeium.sublime-package"
                },
                {
                    "version": "1.2.38",
                    "sublime_text": "*",
                    "url": "https://github.com/Exafunction/codeium-sublime/raw/main/Codeium.sublime-package"
                }
            ]
        }
    ]
}

@fortenforge
Copy link
Contributor Author

I'd like to do a closed-source package not because I'm worried about monetization or source code exposure but because the package currently only works properly with generated python protobuf files and I did not want to have to commit these to the repo.

If the "Manual Hosting" route is not an option and is a blocker for getting this on package control, we can go with the GitHub hosting route instead, just let me know.

Re the other comments:

google.tar.gz and requests.tar.gz are unused.

I'll remove these.

Not sure whether this will work for package updating.

Can you clarify what is wrong? Is it just the wrong order? I can fix that, but is there anything else?

@braver
Copy link
Collaborator

braver commented Jun 22, 2023

The pre-packaged solution is not a problem in and of itself. It does prevent users from seeing what goes on inside the package (default settings, key bindings, any other inner working), and providing feedback or contributions. As you're currently proposing it, users will only be able to find the package by name directly. There will not be any content for it on the package control site, no description or labels that helps users find the package and understand what it's for. There is not even a readme or anything that will tell users what the package does or how it can be used. Or even so much as a place for users to report issues. Even searching the web for "Exafunction codeium" doesn't tell me much about what it is or what it does.

We might go on and review the package contents later, but as it stands, just the proposed entry in the package control registry is not sufficient.

@TerminalFi
Copy link
Contributor

TerminalFi commented Jun 22, 2023

@fortenforge

Installing this in Safe Mode for Sublime and it freezes ST completely.

My assumption is that it is because you are downloading the language server in the main thread which blocks the UI.

This should likely take advantage of threading or a similar to background this tasks. LSP does a great job at doing this in a none blocking way for its language servers.

    @classmethod
    def setup(cls):
        cls.download_server()
        cls.run_server()
sublime_text:
  build: 4150
  channel: dev
  platform: 
    system: MacOS
    arch: arm64
    portable: false
  pre_session_restore_time: NA
  startup_time: NA
  first_paint_time: NA
  env_vars: '/bin/zsh -l'
system_info:
  hardware: MBP 14inch 2022, 10-Core M1 Max, 64 GB, 512GB, Apple M1 Max
  software: macOS 13.3 (22E252)
  kernel_version: Darwin 22.5.0
  open_gl_context_information:
    gl_api_version: 4.1 Metal - 83.1
    glsl_version: 4.10
    vendor: Apple
    renderer: Apple M1 Max

@rwols
Copy link
Contributor

rwols commented Jun 23, 2023

If it’s “just” a language server, consider depending on the LSP package.

@jfcherng
Copy link
Contributor

Not sure whether this will work for package updating.

Can you clarify what is wrong? Is it just the wrong order? I can fix that, but is there anything else?

Iirc, Package Control uses the first match from releases. In your case, it always matches the oldest version. But I didn't read the code, so maybe I am wrong.

@fortenforge
Copy link
Contributor Author

It's not a traditional language server (in particular it doesn't actually implement the language server protocol). Your point about freezing the UI is well-taken though. I'll fix this and let you know once I've done so.

Regarding Manual Hosting vs GitHub Hosting, I took a look at the Package Control link for a manually hosted package: https://packagecontrol.io/packages/Tinkerwell

If I add labels, a homepage, and a description would that be sufficient?

@braver
Copy link
Collaborator

braver commented Jun 24, 2023

If I add labels, a homepage, and a description would that be sufficient?

Yes, that should work. The labels and description would help users find the package, and I would assume the homepage would cover everything else that users need around how to use the package, reports issues, etc. It would be even better if you could provide links for “readme” and “issues” too of course.

@FichteFoll
Copy link
Collaborator

FichteFoll commented Jun 24, 2023

It would be even better if you could provide links for “readme” and “issues” too of course.

Adding a readme link is strongly recommended as that is what the packagecontrol.io site will render for the package.


Further feedback after taking a look at the source:

  • As @jfcherng has pointed out, since you're using the same URL for multiple versions, PC can also make no guarantee about the actual version that ends up being installed because it will depend on what exactly resides at this URL when the package is downloaded. This is not an issue for the tag release workflow because PC grabs a tag's zipball and that would only change if someone deleted and repushed the same tag using a different commit.

  • With this in mind and considering the requirement of including generated protobuf Python files, you could instead host the code natively on the repo but use a Github Action to build an artifact with any potentially necessary build steps and simply reference that from your custom packages.json (which you could also automatically update using a Github Action). Unfortunately, I do know an example repository where such a setup is used (though I know it exists).

  • I see that you're including the requests library inside your package. If your code works on Python 3.3, you could instead also tell PC to install it as a dependency, which can then be shared among multiple packages. If you want to use the 3.8 host however, that is currently not an option as support for dependencies (or "libraries", as they will be called in the future) in the 3.8 host is still in beta.

  • Your fix_imports.py script is a build utility and should not be included at the root level of the package as ST will import and execute it. Now, it likely wouldn't cause issue, since it attempts to iterate over the file system starting from the current working directory, which for ST will usually be its installation folder, but it's unnecessary regardless. Please either move it to a subdirectory or don't include it at all.

  • In language_server.py, you have a hardcoded URL to "https://server.codeium.com" that is passed as a parameter to the LSP-like binary that had been fetched previously. Please clearly mention the kind of remote communication that happens here and is required for the plugin in your readme. There are also a few debug prints in that file.

  • Please prefix the commands that you define with a unique name. Since the commands of all plugins share the same namespace, a very generic name like request_completion could be highly contested otherwise.

  • I didn't dive too much into the completion logic, but I see that you make use of phantoms there. Is there a speicifc reason why you didn't choose to use the existing on_query_completions hook?

  • If your setting for whether the plugin's key bindings should be active is a simple boolean, you could use the settings.<setting_name> context key instead of your custom Codeium.setting key that requires a plugin implementation. I see it's not mentioned by the official documentation, but the community documentation mentions it.

@fortenforge
Copy link
Contributor Author

Alright a list of updates:

  • I removed the tar.gz files and the fix_imports.py script from the .sublime-package
  • I fixed the ordering of the versions in packages.json. Newer versions are now listed first.
  • I added labels, a homepage, a description, a readme, and an issues link to packages.json
  • We do depend on using python 3.8 (protobuf requires 3.8), so we need the 3.8 version of requests.
  • Removed some prints in the source code
  • No longer do we block the UI on language server download
  • Prefaced commands and settings with codeium
  • Mentioned in the README that we communicate with "https://server.codeium.com/"
  • I looked into the on_query_completions hook, but it looks like it triggers less often than the hook we are using; would prefer that our users get completion suggestions at a higher rate.
  • Switched to settings.<setting_name>

If you could give it another round of feedback, that would be much appreciated.

@rchl
Copy link
Contributor

rchl commented Jun 28, 2023

  • I looked into the on_query_completions hook, but it looks like it triggers less often than the hook we are using; would prefer that our users get completion suggestions at a higher rate.

Create and return CompletionList from on_query_completions with DYNAMIC_COMPLETIONS flag set and it should re-query while user is typing.

channel.json Outdated Show resolved Hide resolved
@FichteFoll
Copy link
Collaborator

FichteFoll commented Jun 28, 2023

  • since you're using the same URL for multiple versions, PC can also make no guarantee about the actual version that ends up being installed because it will depend on what exactly resides at this URL when the package is downloaded. This is not an issue for the tag release workflow because PC grabs a tag's zipball and that would only change if someone deleted and repushed the same tag using a different commit.

Also, a quick hint already that this is still an issue. You added a new release triple but all releases point to the exact same upstream URL, making them essentially equivalent. Now, PC doesn't support installing older releases anyway (at the moment), but this could also mean that in the event that you replaced the archive and did not change the version in your packages.json, PC would install the contents of the newer version while thinking it is of an older version. Another issue could be caching if not implemented properly, though I'd hope that GitHub's cache control and HTTP 304 checks are proper. For general safety, I strongly recommend using different URLs for different package versions. If an automatic build process isn't set up, using a commit hash for a specific revision of the file would already help.

Also also, I would also still strongly recommend using a code repository as your basis and building the package archive in an automated fashion, for example using a Github Action, as that will improve the ability to invite contributors and transparency about the package's contents. (Admittedly, that can still be tampered with, but it's a sign of good faith I'd say.) From experience, I can say that helping people with issues in their local setup and a specific package is much easier if the package's contents are readily available for viewing and you don't need to manuall download and extract them.

And a last tip, you can still use the details key to have PC infer most of your keys (issues, readme, homepage, description) automatically and only override the ones that you need to, including the releases.

@fortenforge
Copy link
Contributor Author

  • alphabetized channel.json
  • made all the URLs in package.json unique based on the tag
  • Used details to auto-populate most keys

I looked into on_query_completions again; I'm not sure it's applicable to what we're trying to do. It looks like on_query_completions is used for single-line completions rendered in the intellisense / LSP menu, but this is ill-suited for Copilot/Codeium completions which are (1) often multi-line, (2) quite long, and (3) can sometimes be interleaved with existing text. That's why we're using the phantoms.

@fortenforge fortenforge closed this Jul 3, 2023
@fortenforge
Copy link
Contributor Author

(Ugh, I didn't mean to close this)

@fortenforge fortenforge reopened this Jul 3, 2023
@fortenforge
Copy link
Contributor Author

Hi all, just checking in to see if there's anything else you want me to do here. Thanks for all the feedback you've provided so far!

@FichteFoll
Copy link
Collaborator

Hi, I haven't had the time to look at it again but intend to do so over the weekend.

@FichteFoll
Copy link
Collaborator

FichteFoll commented Jul 9, 2023

Looks good to me. Following are just some comments/recommendations you could also implement but are not required to have this merged. In fact, I'll do so now.

  • There are a couple debug prints that you may want to gate behind a Codeium.debug settuing or similar.
  • Prefer using relative imports when you're importing another module from within your package as those will continue to work when the package folder is renamed.
  • codeium_dir = os.path.join(os.path.expanduser("~"), ".codeium/sublime") is almost equivalent to codeium_dir = os.path.expanduser("~/.codeium/sublime") with the difference that you're not replacing the first forward-slash with the OS-specific path segment separator. However, since you're using the 3.8 host, I'd generally recommend using pathlib.Path anyway, i.e. codeium_dir = Path.home() / ".codeium" / "sublime". Admittedly, that will not work on ST3 obviously unless you also request the pathlib dependency.
  • You have quite a few unused imports (and some variable assignments) in your code. I suggest doing a quick linter pass using flake8.

@FichteFoll FichteFoll merged commit 347f9cb into wbond:master Jul 9, 2023
1 of 2 checks passed
@fortenforge
Copy link
Contributor Author

fortenforge commented Jul 12, 2023

Thanks for merging. Do you know when I can expect Package Control to get updated with it? I don't see Codeium in the Channel json yet and I just wanted to make sure that there was nothing else I needed to do.

@FichteFoll
Copy link
Collaborator

It should definitely be there by now, unless there is a problem with the file that I can't spot with my eyes alone. There is no entry in the cache for https://raw.githubusercontent.com/Exafunction/codeium-sublime/main/packages.json.

@braver, do you have an idea?

@braver
Copy link
Collaborator

braver commented Jul 13, 2023

Nope, sorry, not a clue. We've had cases where @wbond had to go in and manually vet something. Hope it's nothing like that though.

@fortenforge
Copy link
Contributor Author

Hmm, is there any way you could reach out to wbond? I'd really like to resolve this...

@wbond
Copy link
Owner

wbond commented Jul 17, 2023

Sorry for the delay in this - hoping to look into it tonight

@fortenforge
Copy link
Contributor Author

Did you get a chance to look into this?

@wbond
Copy link
Owner

wbond commented Jul 19, 2023

I’ve spent a couple of times over the past 48 hours debugging but haven’t figured it out yet. I believe it is a bug in the crawler - something to do with the package data

@fortenforge
Copy link
Contributor Author

I see that the crawler is open source, if you can give me a couple pointers I can look into it as well if that's helpful

@wbond
Copy link
Owner

wbond commented Jul 21, 2023

The packages.json file at https://raw.githubusercontent.com/Exafunction/codeium-sublime/main/packages.json is missing the date field for each release. I guess the package checker somehow missed that.

It was harder to debug since the package never made in into the DB since it was broken from the start, and thus no page was created upon which to show the error that the date was missing.

@fortenforge
Copy link
Contributor Author

		// If you don't use a "tags" key for a "releases" entry, you need
		// to specify the "version", "url" and "date" manually. Because this
		// requires an update of the repository file for each release it is
		// not allowed in the default repository

Oops my bad. Thanks so much for looking into this! I've added a date field; let's see if this fixes things...

@fortenforge
Copy link
Contributor Author

Yay it worked! https://packagecontrol.io/packages/Codeium

@FichteFoll
Copy link
Collaborator

FichteFoll commented Jul 25, 2023

I guess the package checker somehow missed that.

The package checker never ran for this addition because … it didn't work and we don't know why. The setup is still too fragile but I lack the time to work on it, much like everyone else. (Note: that means running the checker entirely via GitHub Actions to make it easier to debug.)

@rchl
Copy link
Contributor

rchl commented Jul 26, 2023

As a remainder, I've already done that and I'm using it in the LSP Package Control channel

https://github.com/sublimelsp/st-package-reviewer-action
https://github.com/sublimelsp/st-schema-reviewer-action

But I suppose it would still require time to switch to it, test and all.

@FichteFoll
Copy link
Collaborator

Yes, the tracking issue is #8050

@fortenforge
Copy link
Contributor Author

My package Codeium briefly was listed as missing because of a bad PR. Could you re-review and approve it?

https://packagecontrol.io/packages/Codeium

@FichteFoll
Copy link
Collaborator

Unfortunately, only @wbond can do this.

@Gabriel-p
Copy link
Contributor

Any updates on this? The package can not be installed

@FichteFoll
Copy link
Collaborator

Package should be available again.

@Gabriel-p
Copy link
Contributor

Can confirm, is up and running. Thank you!

@BorisLepage
Copy link

I confirm too, thx for the work

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

Successfully merging this pull request may close these issues.

10 participants