-
Notifications
You must be signed in to change notification settings - Fork 464
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
QS: Migrate to pkgutil for databases #985
base: quantumstrand
Are you sure you want to change the base?
Conversation
overall this appears to add complexity vs. reducing it. can you walk me through the benefits of this approach? |
Profiling load_databases:
Also pyinstaller .spec guide mentions both the approaches without specifing any added benefits [using .spec] @williballenthin could you provide us with your concerns for opening the issue . |
I understand This doesn't really matter for packagers that extract a copy of the module to the file system during loading, like PyInstaller. For these packagers, we can use pathlib to access file system paths reletive to Python module contents. For PyInstaller, pkgutil is probably a small shim over file system operations, anyways. However, there are packagers that don't write module content to disk. They might keep Python byte code in memory, having read it from a compressed resource or even from the network. In this case, we can't rely on using pathlib to access data files. We should use pkgutil instead. But! We might decide that we don't want to support those packagers (which seem to be uncommon) and consider this out of scope. Or, we could follow the spec and be prepared to switch to something other than PyInstaller. Does that make sense? @mr-tz @ooprathamm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should also try to find one of these packagers and see if it works with the proposed changes. I suspect Nuitka might work for this.
floss/qs/db/expert.py
Outdated
@@ -51,13 +49,13 @@ def query(self, s: str) -> Set[str]: | |||
return ret | |||
|
|||
@classmethod | |||
def from_file(cls, path: pathlib.Path) -> "ExpertStringDatabase": | |||
def from_file(cls, package: str, resource: str) -> "ExpertStringDatabase": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's keep from_file
around since it's really useful for dev and testing. but let's add a from_pkgutil
to address the feature request. and please factor out the common code into a sub routine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which routine will the get_default_databases
use for now ? Or we intend to keep it as a future utility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please prefer pkgutil
This reverts commit b7d24b5.
For simplicity, the code for using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these changes look great!
I still think we should try to find a package that uses the pkgutil interface. Do you have some time to find one @ooprathamm?
Current Scenario - Other Executable Builder Packages-
Viable Option -
|
first off, great research @ooprathamm! next, the pyoxidizer documentation is outstanding. it does nice job of summarizing the options. and since it doesn't touch on pkgutil, I wonder if we're using that wrong? or is it the new interface alluded to at the end? based on that article, what do you think the best strategy is, @ooprathamm? and, would you consider trying out pyoxidizer on FLOSS to see if it works with our code? |
On further looking into PyOxidizer. The incompatibility is further confirmed here. (comparing different utilities ). For now, It might not be good to consider PyOxidizer |
ok sounds good |
Closes #759