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

Add maximum simultaneous tasks support to TaskContainer #464

Merged

Conversation

cwharris
Copy link
Contributor

@cwharris cwharris commented Apr 4, 2024

Description

Adds maximum simultaneous tasks support to TaskContainer by only starting new tasks if the number of currently executing tasks is less than the maximum number of simultaneous tasks and starting new tasks as older tasks shut down. this eliminates the need for external semaphores or ticketing systems.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

dagardner-nv and others added 4 commits April 3, 2024 20:13
… possible, and as pybind11::object otherwise (nv-morpheus#455)

* Add an optional `unserializable_handler_fn` callback to `mrc::pymrc::cast_from_pyobject` which will be invoked for any unsupported Python object. Allowing for serializing unsupported object.
ex:
```cpp
    pymrc::unserializable_handler_fn_t handler_fn = [](const py::object& source,
                                                       const std::string& path) {
        return nlohmann::json(py::cast<float>(source));
    };

    // decimal.Decimal is not serializable
    py::object Decimal = py::module_::import("decimal").attr("Decimal");
    py::object o       = Decimal("1.0");
    EXPECT_EQ(pymrc::cast_from_pyobject(o, handler_fn), nlohmann::json(1.0));
```

* Add `JSONValues` container class which is an immutable container for holding Python values as `nlohmann::json` objects if possible, and as `pybind11::object` otherwise. The container can be copied and moved, but the underlying `nlohmann::json` object is immutable.
* Updates `nlohmann_json` from 3.9 to 3.11 for `patch_inplace` method.
* Incorporates ideas from @drobison00's PR nv-morpheus#417 with three key differences:

  1. Changes to the `cast_from_pyobject` are opt-in when `unserializable_handler_fn` is provided, otherwise there is no behavior change to the method.
  2. Unserializable objects are stored in `JSONValues` rather than a global cache.
  3. Does not rely on parsing the place-holder values.

This PR is related to nv-morpheus/Morpheus#1560

Authors:
  - David Gardner (https://github.com/dagardner-nv)
  - Devin Robison (https://github.com/drobison00)

Approvers:
  - Michael Demoret (https://github.com/mdemoret-nv)

URL: nv-morpheus#455
@cwharris cwharris added non-breaking Non-breaking change feature request New feature or request labels Apr 4, 2024
@cwharris cwharris requested review from a team as code owners April 4, 2024 20:42
Copy link
Contributor

@mdemoret-nv mdemoret-nv left a comment

Choose a reason for hiding this comment

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

This is a cleaner solution than I was expecting. Have a couple suggestions on cleaning up the TaskContainer implementation to make it more resilient to race conditions.

cpp/mrc/include/mrc/coroutines/task_container.hpp Outdated Show resolved Hide resolved
cpp/mrc/src/public/coroutines/task_container.cpp Outdated Show resolved Hide resolved
cpp/mrc/src/public/coroutines/task_container.cpp Outdated Show resolved Hide resolved
cpp/mrc/src/public/coroutines/task_container.cpp Outdated Show resolved Hide resolved
cpp/mrc/src/public/coroutines/task_container.cpp Outdated Show resolved Hide resolved
cpp/mrc/src/public/coroutines/task_container.cpp Outdated Show resolved Hide resolved
@cwharris cwharris requested a review from mdemoret-nv April 5, 2024 17:02
Copy link
Contributor

@mdemoret-nv mdemoret-nv left a comment

Choose a reason for hiding this comment

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

Looks better with the try_start_next_task() implementation. Just have the one question about m_size. Approved. Can merge when CI is passing. Just want to understand how it works with one variable.

@mdemoret-nv mdemoret-nv changed the base branch from branch-24.06 to branch-24.03 April 6, 2024 00:24
@mdemoret-nv mdemoret-nv requested a review from a team as a code owner April 6, 2024 00:24
Copy link

codecov bot commented Apr 6, 2024

Codecov Report

Merging #464 (f5b5f24) into branch-24.03 (bd7955e) will increase coverage by 0.30%.
Report is 2 commits behind head on branch-24.03.
The diff coverage is 89.75%.

Additional details and impacted files

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-24.03     #464      +/-   ##
================================================
+ Coverage         73.68%   73.98%   +0.30%     
================================================
  Files               400      402       +2     
  Lines             14275    14411     +136     
  Branches           1113     1133      +20     
================================================
+ Hits              10518    10662     +144     
+ Misses             3757     3749       -8     
Flag Coverage Δ
cpp 70.61% <93.71%> (+0.38%) ⬆️
py 41.24% <1.85%> (-0.51%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
cpp/mrc/include/mrc/coroutines/test_scheduler.hpp 100.00% <ø> (+100.00%) ⬆️
cpp/mrc/src/public/coroutines/test_scheduler.cpp 61.76% <100.00%> (+61.76%) ⬆️
...thon/mrc/_pymrc/include/pymrc/asyncio_runnable.hpp 95.23% <100.00%> (+0.15%) ⬆️
...mrc/_pymrc/include/pymrc/utilities/json_values.hpp 100.00% <100.00%> (ø)
...rc/_pymrc/include/pymrc/utilities/object_cache.hpp 0.00% <ø> (ø)
python/mrc/_pymrc/src/utils.cpp 87.50% <100.00%> (+1.95%) ⬆️
cpp/mrc/src/public/coroutines/task_container.cpp 88.46% <91.17%> (+9.39%) ⬆️
python/mrc/_pymrc/src/utilities/json_values.cpp 87.61% <87.61%> (ø)

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd7955e...f5b5f24. Read the comment docs.

@mdemoret-nv
Copy link
Contributor

/merge

@rapids-bot rapids-bot bot merged commit d64eaa5 into nv-morpheus:branch-24.03 Apr 6, 2024
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request non-breaking Non-breaking change
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants