-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Fixing database bug #18409
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
Fixing database bug #18409
Conversation
Summary of ChangesHello @thaisacs, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request fixes a crash that occurs when the database directory for tuning does not exist. The fix correctly introduces logic to create the directory before attempting to write database files. My review includes two suggestions to improve the implementation: one to use create_directories for better robustness in handling nested directory paths, and another to relocate the <filesystem> include to the source file where it is directly used, which is a better practice for dependency management.
| #include <tvm/tir/schedule/schedule.h> | ||
| #include <tvm/tir/schedule/trace.h> | ||
|
|
||
| #include <filesystem> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve code organization and dependency management, it's better to include <filesystem> directly in src/relax/transform/meta_schedule.cc where it's used, rather than in this public header file. This avoids unnecessarily including it for all consumers of database.h and makes dependencies clearer.
src/relax/transform/meta_schedule.cc
Outdated
| database = Database::Current().value(); | ||
| } else { | ||
| ICHECK(work_dir.has_value()); | ||
| std::filesystem::create_directory(work_dir.value().c_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good fix for the reported issue. For better robustness, consider using std::filesystem::create_directories instead of create_directory. create_directories can create parent directories if they don't exist (e.g., if work_dir is a nested path like a/b/c) and it doesn't produce an error if the directory already exists.
| std::filesystem::create_directory(work_dir.value().c_str()); | |
| std::filesystem::create_directories(work_dir.value().c_str()); |
cb3cbb3 to
7d9495a
Compare
tlopex
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
Before this PR, when I tried to tune a model whose pipeline had 0 trials and the database directory didn't exist yet, I would get the following error:
[14:04:12] /home/thais/Dev/tvm/src/relax/transform/meta_schedule.cc:91: Warning: Creating JSONDatabase. Workload at: ./tuning_logs_alexnet/database_workload.json, Tuning records at: ./tuning_logs_alexnet/database_tuning_record.json
Traceback (most recent call last):
File "/home/thais/Dev/TVM-MetaSchedule-Bench/model-tune.py", line 108, in
tune_model(network_arg, args.dtype, TOTAL_TRIALS, target, work_dir)
File "/home/thais/Dev/TVM-MetaSchedule-Bench/model-tune.py", line 31, in tune_model
mod = relax.get_pipeline(
File "/home/thais/Dev/tvm/python/tvm/ir/transform.py", line 167, in call
return _ffi_transform_api.RunPass(self, mod)
File "python/tvm_ffi/cython/function.pxi", line 758, in core.Function.call
File "", line 0, in tvm::transform::Pass::operator()(tvm::IRModule) const
File "", line 0, in tvm::transform::Pass::operator()(tvm::IRModule, tvm::transform::PassContext const&) const
File "", line 0, in tvm::transform::ModulePassNode::operator()(tvm::IRModule, tvm::transform::PassContext const&) const
File "", line 0, in std::_Function_handler<tvm::IRModule (tvm::IRModule, tvm::transform::PassContext), tvm::transform::__TVMFFIStaticInitFunc4()::{lambda(tvm::ffi::TypedFunction<tvm::IRModule (tvm::ffi::RValueRef<tvm::IRModule, void>, tvm::transform::PassContext)>, tvm::transform::PassInfo)#1}::operator()(tvm::ffi::TypedFunction<tvm::IRModule (tvm::ffi::RValueRef<tvm::IRModule, void>, tvm::transform::PassContext)>, tvm::transform::PassInfo) const::{lambda(tvm::IRModule, tvm::transform::PassContext)#1}>::_M_invoke(std::_Any_data const&, tvm::IRModule&&, tvm::transform::PassContext&&)
File "", line 0, in tvm::transform::__TVMFFIStaticInitFunc4()::{lambda(tvm::ffi::TypedFunction<tvm::IRModule (tvm::ffi::RValueRef<tvm::IRModule, void>, tvm::transform::PassContext)>, tvm::transform::PassInfo)#1}::operator()(tvm::ffi::TypedFunction<tvm::IRModule (tvm::ffi::RValueRef<tvm::IRModule, void>, tvm::transform::PassContext)>, tvm::transform::PassInfo) const::{lambda(tvm::IRModule, tvm::transform::PassContext)#1}::operator()(tvm::IRModule, tvm::transform::PassContext) const
File "python/tvm_ffi/cython/function.pxi", line 914, in core.tvm_ffi_callback
File "/home/thais/Dev/tvm/python/tvm/relax/pipeline.py", line 172, in _pipeline
mod = tvm.transform.Sequential(
File "/home/thais/Dev/tvm/python/tvm/ir/transform.py", line 167, in call
return _ffi_transform_api.RunPass(self, mod)
File "python/tvm_ffi/cython/function.pxi", line 758, in core.Function.call
File "", line 0, in tvm::transform::Pass::operator()(tvm::IRModule) const
File "", line 0, in tvm::transform::Pass::operator()(tvm::IRModule, tvm::transform::PassContext const&) const
File "", line 0, in tvm::transform::SequentialNode::operator()(tvm::IRModule, tvm::transform::PassContext const&) const
File "", line 0, in tvm::transform::Pass::operator()(tvm::IRModule, tvm::transform::PassContext const&) const
File "", line 0, in tvm::transform::ModulePassNode::operator()(tvm::IRModule, tvm::transform::PassContext const&) const
File "", line 0, in std::_Function_handler<tvm::IRModule (tvm::IRModule, tvm::transform::PassContext), tvm::relax::transform::MetaScheduleApplyDatabase(tvm::ffi::Optional<tvm::ffi::String, void>, bool)::{lambda(tvm::IRModule, tvm::transform::PassContext)#1}>::_M_invoke(std::_Any_data const&, tvm::IRModule&&, tvm::transform::PassContext&&)
File "", line 0, in tvm::relax::transform::MetaScheduleApplyDatabase(tvm::ffi::Optional<tvm::ffi::String, void>, bool)::{lambda(tvm::IRModule, tvm::transform::PassContext)#1}::operator()(tvm::IRModule, tvm::transform::PassContext) const [clone .constprop.0]
File "", line 0, in tvm::meta_schedule::Database::JSONDatabase(tvm::ffi::String, tvm::ffi::String, bool, tvm::ffi::String)
File "", line 0, in tvm::meta_schedule::JSONFileReadLines(tvm::ffi::String const&, int, bool)
File "", line 0, in tvm::runtime::detail::LogFatal::~LogFatal() [clone .constprop.0]
File "", line 0, in tvm::runtime::detail::LogFatal::Entry::Finalize()
ValueError: Check failed: (os.good()) is false: Cannot create new file: ./tuning_logs_alexnet/database_workload.json