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

Allow optimum to discover and load subpackages #1894

Merged
merged 3 commits into from
Jun 10, 2024

Conversation

dacorvo
Copy link
Contributor

@dacorvo dacorvo commented Jun 5, 2024

What does this PR do?

This introduces explicitly the concept of optimum subpackages that are loaded "dynamically" by the optimum-cli (and possibly in the future by other high-level abstraction modules that would require backend-specific implementations).

An optimum subpackage is primarily a package under the optimum namespace, but it can be also a module inside the optimum package itself, even if that module does not have its own pyproject.toml or setup.py (as a real namespace package would).

By convention, loading a subpackage means importing a subpackage module available at the root of the package.

This pull-request first adds a load_subpackages helper that will go through the installed namespace packages and try to "load" them.

It then adds a decorator to declare custom subcommands, using the same convention as the one already used in the register files.

This is by using this decorator that each subpackage can add its own subcommands, as it will be called by the optimum-cli when evaluating its commands.

In order to validate the new helpers, the onnxruntime commands are moved inside an onnxruntime/subpackage directory that is explicitly loaded by load_subpackages, but only it the onnxruntime package is available.

This improves the UX when using the optimum-cli:

  • commands declared as subpackages will not disappear if the content of the register directory is wiped-out by a new optimum installation,
  • the onnxruntime commands will only be available if the package is actually installed.

@dacorvo dacorvo changed the title Dynamically load subpackages Explicitly load subpackages Jun 5, 2024
@dacorvo dacorvo force-pushed the dynamically_load_subpackages branch from d47be23 to 35e5e79 Compare June 5, 2024 14:13
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@dacorvo dacorvo force-pushed the dynamically_load_subpackages branch 2 times, most recently from 33cf651 to 70660ad Compare June 5, 2024 15:27
@dacorvo dacorvo changed the title Explicitly load subpackages Allow optimum to discover and load subpackages Jun 6, 2024
@dacorvo dacorvo marked this pull request as ready for review June 6, 2024 06:55
As an alternative to directly adding their commands in a register.py file under
the root optimum directory, this adds a decorator to declare a subcommand that
can be used by subpackages when they are loaded.
This will fix the issue of subcommands 'disappearing' when optimum is upgraded
without reinstalling the subpackage.
The onnxruntime commands are moved into a subpackage loader directory.
This subpackage directory is only loaded (and its commands added) when
the onnxruntime is available.
This avoids wrongly indicating that the onnxruntime commands are available
when the package is actually not installed.
@dacorvo dacorvo force-pushed the dynamically_load_subpackages branch from 70660ad to 40e4f8d Compare June 6, 2024 07:47
Copy link
Collaborator

@fxmarty fxmarty left a comment

Choose a reason for hiding this comment

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

LGTM, apart that optimum-cli has a 1-3s delay to start running (already on main) on my laptop, which should be ideally improved in a future PR

@@ -17,7 +17,7 @@
from pathlib import Path
from typing import TYPE_CHECKING

from .. import BaseOptimumCLICommand
from optimum.commands import BaseOptimumCLICommand
Copy link
Collaborator

Choose a reason for hiding this comment

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

why absolute path?

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's mostly because the relative path would have been complicated, but also to serve as a model for actual subpackages that can live outside the optimum tree.

Comment on lines +75 to +79
SUBPACKAGE_LOADER = "subpackage"
load_namespace_modules("optimum", SUBPACKAGE_LOADER)

# Load subpackages from internal modules not explicitly defined as namespace packages
loader_name = "." + SUBPACKAGE_LOADER
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is SUBPACKAGE_LOADER?

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's the name of the module that is imported in each subpackage to "load" them. I used a constant because I use it twice and was expecting maybe some discussion about the naming.

@@ -15,5 +15,4 @@
from .base import BaseOptimumCLICommand, CommandInfo, RootOptimumCLICommand
from .env import EnvironmentCommand
from .export import ExportCommand, ONNXExportCommand, TFLiteExportCommand
from .onnxruntime import ONNXRuntimeCommand, ONNXRuntimeOptimizeCommand, ONNXRuntimeQuantizeCommand
from .optimum_cli import register_optimum_cli_subcommand
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would need to make sure register_optimum_cli_subcommand is not used in subpackages

Copy link
Contributor Author

@dacorvo dacorvo Jun 6, 2024

Choose a reason for hiding this comment

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

There are no calls in optimum and I quickly checked the optimum-XX to verify that none of them was calling it either, but I was not 100 % sure. If that is an issue I can make it public again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Existing subpackages are using the "register" method at the moment.

Copy link
Member

@michaelbenayoun michaelbenayoun left a comment

Choose a reason for hiding this comment

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

That is super cool!
Thanks for this improvement @dacorvo

Comment on lines +21 to +24
if sys.version_info >= (3, 8):
from importlib import metadata as importlib_metadata
else:
import importlib_metadata
Copy link
Member

Choose a reason for hiding this comment

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

That raises the question for later: what is the minimal python version we want to support. I have seen different things accross differents huggingface libraries.

Choose a reason for hiding this comment

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

Python 3.8 is the oldest supported, EOL is october 2024: https://devguide.python.org/versions/
I guess we could only support 3.8 and newer.

@dacorvo dacorvo merged commit f33f2f1 into main Jun 10, 2024
45 of 47 checks passed
@dacorvo dacorvo deleted the dynamically_load_subpackages branch June 10, 2024 08:06
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

Successfully merging this pull request may close these issues.

None yet

5 participants