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

Efficiency of poetry-call and poetry-do-call #39

Open
calebstewart opened this issue Sep 27, 2022 · 3 comments
Open

Efficiency of poetry-call and poetry-do-call #39

calebstewart opened this issue Sep 27, 2022 · 3 comments

Comments

@calebstewart
Copy link

calebstewart commented Sep 27, 2022

I'd like to preface this with the caveat that I am not an elisp expert. I would barely classify myself as a beginner. That being said, the efficiency of this module has been really bothering me lately. I've disabled poetry-tracking-mode altogether because regardless of the tracking strategy, the sad fact is that executing poetry commands is painfully slow. This is a huge bummer because the idea is fantastic. We should be able to automatically track the appropriate environment smoothly as we move between files.

Because of this, I recently started going through the code to try to figure out why this could be. After all, nothing this package is doing is complicated in anyway. It just needs to traverse the directory tree, find a pyprojec.toml that uses poetry, and then make one or two poetry calls to deduce the environment path. Poetry itself isn't slow, so this should be very quick.

Upon looking at the code, I was baffled by the implementation of these two functions. Namely, the synchronization between the two of them and the synchronization with the subprocess itself. Especially the use of sleep-for .1. I had some thoughts after doing some research around Emacs/elisp documentation, and I'm curious of anyone's thoughts. I'm not sure if I'll have the time to get up to speed in Elisp and this project to actually implement them, but wanted to raise some options to maybe help the project along.

  1. The poetry-do-call function uses a polling loop to wait for process completion. Could this not be replaced by a simple (while (accept-process-output process)) loop as shown here?
  2. The poetry-call function uses a similar polling loop when in blocking mode to wait for the queue to empty in order to call the requested poetry command. Could we not use a condition variable here to remove the polling loop? Reference. In reality, this function essentially forces all poetry-call invocations to be serialized (i.e. non-parallel), so you could even just wrap the call in a mutex and get rid of the queue altogether unless I'm misunderstanding something.
  3. All of the blocking semantics defined here seem to be in an attempt to keep the Poetry output buffer tidy. I'm not intimately familiar with how buffers work from the perspective of plugins and Elisp, but is it possible to have a separate poetry-call function for internal calls which uses a temporary buffer? My thought-process here is that the Poetry buffer is mainly useful when doing manual operations like adding a dependency, updating the lock file, packaging your application, etc. However, there's no need for this song-and-dance when we are updating the virtual environment. Those commands have no need to fuss with any kind of synchronization or save their output to a persistent buffer that I can think of. For example, the call to poetry info -p in poetry-get-virtualenv has no need to put it's output in the persistent Poetry buffer and could instead be executed on a temporary/ephemeral buffer with no locking or queue semantics. I'm happy to be proven wrong here, but it's just a thought.

Again, just to reiterate, I'm not an elisp dev. I'm just a happy Emacs user, and I'd love to see this project improve. I love poetry, and I don't want to use any editor other than Emacs if I can avoid it. :)

@calebstewart
Copy link
Author

In case anyone else ends up here, if you are using Doom Emacs, and want to disable poetry-tracking-mode but still provide an easy way to switch between environments, this works for me. I explicitly clear out the internal poetry variables so that the module will deduce my current project again. This is good for me, since I am working out of a monorepo at the moment which houses a few different Python packages under one logical project. I can now just press SPC p u to automatically switch to whatever Poetry environment the current buffer lives in.

For anyone who doesn't use Doom, the advice-add isn't necessary. Doom has decided to hard-code a python-mode-hook which enabled poetry-tracking-mode when +poetry is enabled. This is the best way I've found to stop it from happening since it's hardcoded into Doom.

; Nuke poetry tracking mode from orbit
(advice-add 'poetry-tracking-mode :override (lambda () ()))

(defun custom-reload-venv ()
  "Update Python Poetry Virtual Environment"
  (interactive)

  ; Deactivate the current environment
  (pyvenv-deactivate)

  ; Reset internal poetry configs to trigger a reload of the current project
  (setq poetry-project-venv nil)
  (setq poetry-project-root nil)
  (setq poetry-project-name nil)
  (setq poetry-saved-venv nil)

  ; Re-deduce our project directory and venv, and activate it.
  (poetry-venv-workon)

  ; Reload language server to use the new venv
  (lsp-restart-workspace)
)

(map! :leader
      :desc "Update Python Poetry Virtual Environment"
      "p u" 'custom-reload-venv)

@aacunningham
Copy link

Maybe as a separate anecdote, I decided to give doom emacs a spin after repeated pitches from a colleague last Thursday. I was really enjoying it up until the point I discovered how to add enable poetry support, and was pretty dismayed at the performance and thought about putting it down.

I've got essentially zero experience with elisp, but I'd love to help resolve some of these issues any way I can. Gonna spend my time reading through the code and getting more familiar, and then I'd love to dig more into this and help speed up the package if possible.

@aacunningham
Copy link

aacunningham commented Oct 4, 2022

Alright, so kinda just spitballing here. Been reading the code, reading docs on some of the various functions called, and skimming enough of the Processes section of the elisp documentation to fit my agenda. As far as I understand it, I'm still not 100% sure I understand why we're using compile to run the process, as opposed to something like call-process or make-process.

I need to understand more of the code, but I'm tempted to swap out the code that makes poetry processes and replace it with the synchronous call-process. If that improves the speed of the tracking mode, then maybe it'd also be worthwhile attempting the async method and using sentinels? idk still learnin, someone hop in if I'm in the completely wrong direction.

BTW, that snippet to disable the tracking mode and the new shortcut have been super useful for the time being, works great for me.

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

No branches or pull requests

2 participants