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.

Sign up for GitHub

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

Only reload cache once for rope_autoimport #463

Closed
wants to merge 2 commits into from
Closed

Only reload cache once for rope_autoimport #463

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
44225c0
rope_autoimport - only reload cache once
tkrabel-db Oct 16, 2023
d265021
fix pylint
tkrabel-db Oct 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions pylsp/plugins/rope_autoimport.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
_score_pow = 5
_score_max = 10**_score_pow
MAX_RESULTS = 1000
ENABLED = False


@hookimpl
Expand Down Expand Up @@ -203,6 +204,8 @@ def _sort_import(score: int) -> str:
def _reload_cache(
config: Config, workspace: Workspace, files: Optional[List[Document]] = None
):
# pylint: disable=global-statement
global ENABLED
memory: bool = config.plugin_settings("rope_autoimport").get("memory", False)
rope_config = config.settings().get("rope", {})
autoimport = workspace._rope_autoimport(rope_config, memory)
Expand All @@ -214,6 +217,7 @@ def _reload_cache(
)
autoimport.generate_cache(task_handle=task_handle, resources=resources)
autoimport.generate_modules_cache(task_handle=task_handle)
ENABLED = True


@hookimpl
Expand Down Expand Up @@ -241,12 +245,14 @@ def pylsp_document_did_save(config: Config, workspace: Workspace, document: Docu


@hookimpl
def pylsp_workspace_configuration_chaged(config: Config, workspace: Workspace):
def pylsp_workspace_configuration_changed(config: Config, workspace: Workspace):
"""
Initialize autoimport if it has been enabled through a
workspace/didChangeConfiguration message from the frontend.

Generates the cache for local and global items.
"""
if config.plugin_settings("rope_autoimport").get("enabled", False):
if config.plugin_settings("rope_autoimport").get("enabled", False) and not ENABLED:
_reload_cache(config, workspace)
else:
log.debug("autoimport: Skipping cache reload.")
Comment on lines +255 to +258
Copy link
Contributor

@rchl rchl Oct 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

In a situation when it's disabled and then re-enabled again later we should reload the cache. So we should change ENABLED to false in the else case.

BTW. I wasn't aware before that _reload_cache is called quite often (on file open and save). Knowing that, I suppose this is not really that much of a performance issue really... Sorry for not checking it properly before

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I checked it myself and this indeed can be a performance issue. On a larger python venv with all the latest data science packages like pandas, sklearn and the sort, we have ~130 python packages in that venv, which takes 30sec for rope to scan if memory = True. That said, I see future work to make _reload_cache async, so that the language server is unblocked and can handle other requests while loading the cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, thinking about this more, I start doubting the value of this PR. workspace/didChangeConfiguration is something I expect a user to set once during the session, not so often that it's worth adding a state machine to rope_autoimport now. And the inefficiencies won't be an issue anyway when rope is disabled. Also, when rope creates an index DB, reloading the cache is fast (~1 sec on 130 dependencies).
On the other hand, the state machine added would get fairly complicated, making it harder to reason about the logic, and as code is more often read than written, I'm hesitant to add it just now. Let's wait for user feedback first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That said, I open a separate PR to fix the obvious bug in the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

I expect a user to set once during the session, not so often that it's worth adding a state machine to rope_autoimport now

It would be buggy without it. The user would disable cache and on enabling it later the cache would be stale.

Maybe a corner case but it would still result in incorrect behavior so I think we shouldn't ignore it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, sorry, you meant it's an option to drop this whole PR rather than not addressing my comment. Yeah, that's probably fine too given how often the cache is reloaded anyway.

Copy link
Member

Choose a reason for hiding this comment

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

workspace/didChangeConfiguration is something I expect a user to set once during the session, not so often that it's worth adding a state machine to rope_autoimport now

Actuallly, this can happen several times in a single session (at least in Spyder) because users can enable/disable pycodestyle, pydocstyle or change their preferred auto-formatter provider in Spyder Preferences. And all that involves a didChangeConfiguration.

Hm, thinking about this more, I start doubting the value of this PR.
Also, when rope creates an index DB, reloading the cache is fast (~1 sec on 130 dependencies).

Ok, so do you prefer to close this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, let's close this