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

Autoimport autodetection #516

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

bagel897
Copy link
Contributor

@bagel897 bagel897 commented Nov 3, 2022

Motivation

Say you have a large environment with a package (IE: Pytorch) that you mark as a dependency. While you plan to use pytorch, its dependents consume a lot of unnessecary space. Additionally, it doesn't make sense to directly import those packages (since it breaks abstraction).

Description

Autodetects dependencies
This makes autoimport much much faster (1.33 sec vs 0.28 sec on a large env)
Pyproject.toml support
Will deprecate setting underlined per-function/directly w/o going through prefs (feedback?)

Checklist (delete if not relevant):

  • I have added tests that prove my fix is effective or that my feature works
  • I have updated CHANGELOG.md
  • I have made corresponding changes to user documentation for new features
  • I have made corresponding changes to library documentation for API changes

@lieryan
Copy link
Member

lieryan commented Nov 29, 2022

@bagel897 other than Database versioning, is there any other things that this PR will need to resolve before this draft PR is "ready for review"?

@bagel897
Copy link
Contributor Author

bagel897 commented Nov 29, 2022

I'm going to be very sporadic on all PRs, sorry.
I want to deprecate the underlined setting and make it specified in toml but am unsure of the logistics - we need a way to recreate dbs with new settings and versioning and remove all the existing arguments to functions. It might be too ambitious for little gain, though

@lieryan
Copy link
Member

lieryan commented Nov 29, 2022

No worries, if you are not already working on it, I might pick the task for that database schema versioning so the DB versioning probably should have its own ticket.

@bagel897 bagel897 marked this pull request as ready for review December 6, 2022 18:56
@bagel897
Copy link
Contributor Author

bagel897 commented Dec 6, 2022

I'll move the remaining stuff to future PRs

@bagel897
Copy link
Contributor Author

bagel897 commented Apr 18, 2023

@lieryan Sorry for the miscommunication - this PR should be ready.
Update: Never Mind, I'll fix this later

@bagel897 bagel897 marked this pull request as draft November 2, 2023 18:33
@bagel897 bagel897 marked this pull request as ready for review January 9, 2024 19:12
@bagel897
Copy link
Contributor Author

bagel897 commented Jan 9, 2024

@lieryan I finally got this one ready, thoughts?

Copy link
Member

@lieryan lieryan left a comment

Choose a reason for hiding this comment

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

Thanks @bagel897, excellent as usual. There are a couple notes here that should be looked into.

ropetest/contrib/autoimport/deptest.py Outdated Show resolved Hide resolved
ropetest/contrib/autoimport/deptest.py Outdated Show resolved Hide resolved
rope/base/prefs.py Outdated Show resolved Hide resolved
project_package = get_package_tuple(project.root.pathlib, project)
assert project_package is not None
assert project_package.path is not None
if underlined is not None:
self.prefs.underlined = underlined
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to copy the self.prefs here if we're going to mutate it? Otherwise it would change the setting globally and in other instances of AutoImport that shares the same prefs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point - I ran a deep copy. Maybe in the future, it should automatically reload on pyproject.toml changes? Would you want me to include that in this PR?

underlined: bool = field(
default=False, description="Cache underlined (private) modules")
memory: bool = field(default=None, description="Cache in memory instead of disk")
parallel: bool = field(default=True, description="Use multiple processes to parse")
Copy link
Member

@lieryan lieryan Jan 30, 2024

Choose a reason for hiding this comment

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

Thinking about this further on the config. A couple thoughts:

  • I don't think parallel should be user-configurable. I'd have preferred to not make parallelization configurable at all, but if there's use case single-threaded mode, I think it's generally the editor/IDE integrators that would need to choose the best value for their integration, not the user. It's generally going to be a matter of whether their environment supports/allows multithreading.

    • Maybe we can make some of these hidden/advanced settings, or add warnings that users generally shouldn't be toggling them unless directed by a rope/integration developer
  • I'm not quite sure whether memory should be global or autoimport-specific configuration. While this setting currently only affects autoimport, we may want to expand this to other caches. Users that don't want rope to create extra files in the project directory likely would want to just turn off all cached files and users that are fine with .ropeproject folder mostly wouldn't care about the different cache files that rope generates.

    • It's only a small amount of very advanced users or users that are having very specific problems that would've cared to vary the memory per internal rope component. Unless there's a strong use case for per-component fine tuning here, I would rather keep things that can be configured to a minimum to keep maintenance simpler.

@@ -14,6 +14,8 @@ Will be used if [tool.rope] is configured.

[tool.rope]
split_imports = true
[tool.rope.autoimport]
underlined = false
Copy link
Member

@lieryan lieryan Jan 30, 2024

Choose a reason for hiding this comment

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

I'd much prefer to keep a flat namespace here for the config keys. The vast majority of users are going to only have a small amount of rope config, creating multiple config sections just for a handful of configuration seems overkill and creates opportunities for errors. Most users wouldn't care/know about rope internals enough to intuitively know that autoimport is a separate package inside rope.

Copy link
Member

Choose a reason for hiding this comment

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

IIRC, toml allows dotted keys. Maybe we can have our cake and eat it too, by recommending the dotted form in the examples section instead of the separate section:

[tool.rope]
autoimport.underlined = true

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.

2 participants