Task variant registry #675
Conversation
This reverts commit a0bf8ec.
magnatelee
added
the
category:improvement
|
@bryevdv Note that I ignored the typing error from the new Cython module and I haven't figured out a way to fix it. |
src/core/runtime/context.h
Outdated
| @@ -85,6 +100,7 @@ class ResourceScope { | |||
| { | |||
| return base_ <= resource_id && resource_id < base_ + max_; | |||
| } | |||
| int64_t max() const { return max_ - 1; } | |||
There was a problem hiding this comment.
The naming of max is a bit confusing here. I would definitely rename the member variable max_ to something like num_ids and change the function from max() to max_valid_id().
You could possible even change ResourceScope to ResourceIdScope.
src/core/task/variant_helper.h
Outdated
| }; | ||
|
|
||
| template <typename T, template <typename...> typename SELECTOR, bool HAS_VARIANT> | ||
| struct VariantHelperImpl { |
There was a problem hiding this comment.
I believe you can do this with one fewer template classes and remove the VariantHelperImpl:
template <typename T, template <typename...> typename SELECTOR, bool valid = SELECTOR<T>::value>
struct VariantHelper {
static void record(TaskInfo* task_info,
const std::map<LegateVariantCode, VariantOptions>& all_options)
{
}
};
template <typename T, template <typename...> typename SELECTOR>
struct VariantHelper<T, SELECTOR, true> {
static void record(TaskInfo* task_info,
const std::map<LegateVariantCode, VariantOptions>& all_options)
{
legate/core/_lib/context.pyx
Outdated
| # limitations under the License. | ||
| # | ||
|
|
||
| # distutils: language = c++ |
There was a problem hiding this comment.
Is this required? I believe that specifying CXX in rapids_cython_create_modules is sufficient but I could be wrong.
legate/core/_lib/context.pyx
Outdated
| cdef class Context: | ||
| cdef LibraryContext* _context | ||
|
|
||
| def __init__(self, str library_name, bool can_fail=False) -> None: |
There was a problem hiding this comment.
Better to use __cinit__, rather than __init__ for C++-level initialization of the object:
|
|
||
| cdef extern from "core/task/task_info.h" namespace "legate" nogil: | ||
| cdef cppclass TaskInfo: | ||
| bool has_variant(int) |
There was a problem hiding this comment.
Can the methods of this class (or any of the below classes) raise C++ exceptions? If so, they should be declared with an except + in order for those C++ exceptions to be translated into Python exceptions when they occur. For example:
bool has_variant(int) except +
If not, please feel free to ignore this comment.
See nv-legate/legate#675 and nv-legate/legate#676 Authors: - Mads R. B. Kristensen (https://github.com/madsbk) Approvers: - Lawrence Mitchell (https://github.com/wence-) URL: #202
This PR adds a per-library task registry and uses it to check if a given task launch can be performed successfully. As part of the change, the PR also 1) adds an API to register a task without going through the registrar and 2) refactors the task registration logic so that client libraries no longer need to forward a call back to the core. The PR also adds an integration test to exercise new functionalities.