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

Fix x86 target compilation #49

Merged
merged 2 commits into from
May 1, 2023

Conversation

guillaume-be
Copy link
Contributor

Hello,

I believe recent changes (#48) have broken x86 compilation. The current bindings are currently setting a special case for x86 targets, replacing size_t by usize when generating the bindings:

ort/build.rs

Line 660 in 254a5ce

.size_t_is_usize(env::var("CARGO_CFG_TARGET_ARCH").unwrap().contains("x86"))
resulting in
pub gpu_mem_limit: usize,

This means that sys::OrtROCMProviderOptions expects a usize for the gpu_mem_limit field rather than a c_ulong. The current code does not compile with the following error message:

 gpu_mem_limit,
 ^^^^^^^^^^^^^^ expected usize, found u32

This PR handles the special x86 architecture case, and should hopefully maintain M1 mac compatibility from the previous PR.

This could also be fixed by casting into the expected type with:

let rocm_options = sys::OrtROCMProviderOptions {
        [...]
	gpu_mem_limit: gpu_mem_limit.try_into().unwrap(),
        [...]
};

Please let me know what your preferred solution would be, happy to amend the change request

@codecov-commenter
Copy link

codecov-commenter commented May 1, 2023

Codecov Report

Merging #49 (c691073) into main (254a5ce) will not change coverage.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##             main      #49   +/-   ##
=======================================
  Coverage   48.71%   48.71%           
=======================================
  Files          19       19           
  Lines        1051     1051           
=======================================
  Hits          512      512           
  Misses        539      539           
Impacted Files Coverage Δ
src/execution_providers.rs 5.00% <ø> (ø)

@decahedron1 decahedron1 merged commit 681be5b into pykeio:main May 1, 2023
decahedron1 pushed a commit that referenced this pull request May 1, 2023
* - Fix windows compilation

* Fix lint
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