Skip to content

added cache version for nuphar JIT binaries #2646

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

Merged
merged 3 commits into from
Dec 15, 2019

Conversation

yangchen-MS
Copy link
Contributor

Previously, when the user wrongfully loaded a JIT binary generated
from a Nuphar version different from the current used one, she
would get mysterious runtime failures, because we didn't perform
any version check on JIT binaries.

This change added cache versions to the Nuphar runtime and
JIT binaries. The Nuphar runtime will issue verbose message that
informs the user version-mismatch errors.

Previously, when the user wrongfully loaded a JIT binary generated
from a Nuphar version different from the current used one, she
would get mysterious runtime failures, because we didn't perform
any version check on JIT binaries.

This change added cache versions to the Nuphar runtime and
JIT binaries. The Nuphar runtime will issue verbose message that
informs the user version-mismatch errors.
@yangchen-MS yangchen-MS requested a review from a team as a code owner December 13, 2019 01:16
}
__EOF__
g++ -std=c++14 -fPIC -o cache_version.o -c cache_version.cc
rm cache_version.cc
Copy link
Contributor

@ke1337 ke1337 Dec 13, 2019

Choose a reason for hiding this comment

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

can you have a single loop to compile all cc files, so it would be easier to add cc files in future? #Resolved

@@ -43,7 +37,7 @@ static bool GetOrCreateTVMModuleCacheDirectory(fs::path& path, bool create) {
throw std::runtime_error("Failed to create directory " + path.string());
}

path.append(version);
path.append(kNupharCacheVersion);
Copy link
Contributor

@ke1337 ke1337 Dec 13, 2019

Choose a reason for hiding this comment

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

path.append(kNupharCacheVersion); [](start = 2, length = 33)

Could the version in path be removed? #Resolved

Copy link
Contributor Author

@yangchen-MS yangchen-MS Dec 13, 2019

Choose a reason for hiding this comment

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

Removing version from the path would be a breaking change to users. I think keeping different versions of .dll/.so in separate folders makes the user's life easier. It won't hurt at least.
#Resolved

@@ -251,7 +251,7 @@ endif()

if (onnxruntime_USE_NUPHAR)
file(GLOB onnxruntime_python_nuphar_python_srcs CONFIGURE_DEPENDS
"${ONNXRUNTIME_ROOT}/core/providers/nuphar/scripts/*.*"
"${ONNXRUNTIME_ROOT}/core/providers/nuphar/scripts/*"
Copy link
Contributor

Choose a reason for hiding this comment

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

${ONNXRUNTIME_ROOT}/core/providers/nuphar/scripts/* [](start = 5, length = 51)

Please note that python wheel only takes .py files, and NUPHAR_CACHE_VERSION may not be packed in wheel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, what about create_shared.sh and created_shared.cmd?

Copy link
Contributor

Choose a reason for hiding this comment

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

Those are not included in the python wheel.


In reply to: 357511621 [](ancestors = 357511621)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems we don't need to include create_shared.py (and hence NUPHAR_CACHE_VERSION) either?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me build a python wheel. Just by reading the code, seems to me that all the files under nuphar/scripts are copied into the wheel package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a minor fix to include NUPHAR_CACHE_VERSION into the python wheel

@yangchen-MS
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 12 pipeline(s).

@yangchen-MS yangchen-MS merged commit f741289 into master Dec 15, 2019
@yangchen-MS yangchen-MS deleted the yanchen/nuphar/cache_ver branch December 15, 2019 06:46
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.

2 participants