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

Make the accelerator EP optional for non-onnx pass #1072

Merged
merged 8 commits into from
Apr 24, 2024
Merged

Conversation

guotuofeng
Copy link
Collaborator

Describe your changes

  • Originally, the execution_providers in accelerator spec is required regardless whether it is used or not. Even if user doesn't specify them, the local system will infer and get the available EPs to autofill the EPs. This is painful for passes that doesn't use the EP.
  • In this PR, if there is no pass that belongs to onnx techniques, we will not infer the EP based on the device.
  • On the other hand, if the device is not specified, we will choose cpu by default.

Checklist before requesting a review

  • Add unit tests for this change.
  • Make sure all tests can pass.
  • Update documents if necessary.
  • Lint and apply fixes to your code by running lintrunner -a
  • Is this a user-facing change? If yes, give a description of this change to be included in the release notes.
  • Is this PR including examples changes? If yes, please remember to update example documentation in a follow-up PR.

(Optional) Issue link

olive/hardware/accelerator.py Outdated Show resolved Hide resolved
olive/systems/utils/misc.py Show resolved Hide resolved
olive/workflows/run/run.py Show resolved Hide resolved
olive/hardware/accelerator.py Show resolved Hide resolved
@trajepl
Copy link
Contributor

trajepl commented Apr 11, 2024

Could you help add TODO item to the example config? For example, llama-qlora.json
image

We can try to update the example config in a separated PR.

@guotuofeng
Copy link
Collaborator Author

Could you help add TODO item to the example config? For example, llama-qlora.json image

We can try to update the example config in a separated PR.

json could not add comments

trajepl
trajepl previously approved these changes Apr 12, 2024
@xiaoyu-work
Copy link
Contributor

Can you also remove the EP section from our examples for those which don't need EP?

olive/hardware/accelerator.py Dismissed Show dismissed Hide dismissed
olive/hardware/accelerator.py Dismissed Show dismissed Hide dismissed
@guotuofeng guotuofeng merged commit 3de60e0 into main Apr 24, 2024
35 checks passed
@guotuofeng guotuofeng deleted the myguo/ep_op branch April 24, 2024 02:29
jambayk added a commit that referenced this pull request Apr 26, 2024
…ecution_provider (#1116)

## Describe your changes
This fixes some bugs introduced by some recent PRs:
- #1091
- made `model.io_config` a property which calls `get_hf_model_io_config`
if the conditions meet.
- However, this function fails if the model is not supported. Now it
doesn't fail and logs when unsupported.
- `PytorchModelHandler.to_json` uses `_io_config` for `"io_config"` so
that the serialized model is the same as the one that was initialized.
- #1072
- execution_provider is optional for non-onnx pass but `to_json` still
has the key. Which doesn't match the signature for
https://github.com/microsoft/Olive/blob/91c4c20e324fbc47dcbbea20bf89035ec94fa4a6/olive/passes/olive_pass.py#L459
   - Only add when not None
- Also removed `pass_accelerator_type` and `pass_execution_provider`
from the aml_pass_runner command line inputs. These values are already
present in `pass_config.json`
 
## Checklist before requesting a review
- [ ] Add unit tests for this change.
- [ ] Make sure all tests can pass.
- [ ] Update documents if necessary.
- [ ] Lint and apply fixes to your code by running `lintrunner -a`
- [ ] Is this a user-facing change? If yes, give a description of this
change to be included in the release notes.
- [ ] Is this PR including examples changes? If yes, please remember to
update [example
documentation](https://github.com/microsoft/Olive/blob/main/docs/source/examples.md)
in a follow-up PR.

## (Optional) Issue link
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

3 participants