Only reload cache once for rope_autoimport
#463
Conversation
| if config.plugin_settings("rope_autoimport").get("enabled", False) and not ENABLED: | ||
| _reload_cache(config, workspace) | ||
| else: | ||
| log.debug("autoimport: Skipping cache reload.") |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That said, I open a separate PR to fix the obvious bug in the code.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yes, let's close this
ccordoba12
changed the title
rope_autoimport
Addressed #461 (comment) and a minor bug.