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

[tuner] Add device validation for user devices #60

Merged

Conversation

mihaescuvlad
Copy link
Contributor

The input will be verified against the available devices on the user's machine.

The old implementation of --devices was taking in an integer, it will now take in a string representing the device's Path / UUID.

  • Adds: fetch_available_devices() -> list[any] - Returns the list of all of the devices found by the IREE Runtime module
  • Adds: validate_devices(user_devices) -> None - Takes in the user input and checks it against the available devices and throws an error if at least one of the user's devices is invalid.
  • Modified: parse_devices(devices_str: str) -> list[str] - The documentation of the method has been updated and validate_devices will be called to validate the user input
  • Modified: parse_arguments() - The help message has been updated to better represent what the new input format for --devices is expected to look like
  • Fixed typo: Bnechmarking -> Benchmarking

@kuhar kuhar requested review from kuhar and RattataKing July 17, 2024 06:00
Copy link
Member

@kuhar kuhar left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Left some comments.

The biggest issue right now is that this does not seem to allow for device IDs being used (e.g., hip://0).

tuning/autotune.py Outdated Show resolved Hide resolved
tuning/autotune.py Outdated Show resolved Hide resolved
tuning/autotune.py Outdated Show resolved Hide resolved
tuning/autotune.py Outdated Show resolved Hide resolved
tuning/autotune.py Outdated Show resolved Hide resolved
tuning/autotune.py Outdated Show resolved Hide resolved
tuning/autotune.py Outdated Show resolved Hide resolved
Copy link
Contributor

@RattataKing RattataKing left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! ᕕ( ᐛ ) ᕗ

Copy link
Member

@kuhar kuhar left a comment

Choose a reason for hiding this comment

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

Looks good, just a few small issues

tuning/autotune.py Outdated Show resolved Hide resolved
tuning/autotune.py Outdated Show resolved Hide resolved
tuning/autotune.py Show resolved Hide resolved
Copy link
Member

@kuhar kuhar left a comment

Choose a reason for hiding this comment

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

One more thing: we should also update the benchmark_*.sh files so that they expect full device URIs. Currently they prepend hip:// on their own.

@mihaescuvlad mihaescuvlad force-pushed the users/mihaescuvlad/add-devices-validation branch from 432261b to 4e5c139 Compare July 24, 2024 18:04
@kuhar kuhar changed the title Add device validation for user devices [tuner] Add device validation for user devices Jul 24, 2024
Copy link
Member

@kuhar kuhar left a comment

Choose a reason for hiding this comment

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

Looks good! Could you add a couple of test cases for extract_driver_name and parse_devices to test_autotune.py? And also address this: #60 (review).

Initial changes did not fully commit

Optimized validation & implemented CR suggestions

Fix merge issues

Add device validation for user devices

Initial changes did not fully commit

Optimized validation & implemented CR suggestions

Add device validation for user devices

Optimized validation & implemented CR suggestions

Fix merge issues

Add device validation for user devices

Optimized validation & implemented CR suggestions

Add unit tests for autotuning

Update benchmark scripts to take in any device
@mihaescuvlad mihaescuvlad force-pushed the users/mihaescuvlad/add-devices-validation branch from 95ef174 to 72c58a6 Compare August 5, 2024 17:54
@kuhar kuhar merged commit fa34a1b into nod-ai:main Aug 5, 2024
kuhar pushed a commit that referenced this pull request Aug 6, 2024
Fix new lint errors for #60
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.

3 participants