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

Adding Dynamic Selection RFC #2079

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

vossmjp
Copy link
Contributor

@vossmjp vossmjp commented Feb 25, 2025

A description of the Dynamic Selection experimental feature. The intent is to capture the current design so that future RFCs can propose changes.

Copy link
Contributor

@danhoeflinger danhoeflinger left a comment

Choose a reason for hiding this comment

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

Mostly minor comments.
I think this is a nice distillation.

Comment on lines 63 to 68
// (4) each submission can be waited on using the returned object
ex::wait(done);
}

// (5) and/or all submissions can be waited on as a group
ex::wait(p.get_submission_group());
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to make more clear that in a normal case, one would use 4 or 5, rather than a combination of both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 82 to 83
to the Backend. The handle `h` is the mechanism for the Backend to report runtime
information required by the Policy logic for making future selections.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to mention here a situation where runtime information being reported to the policy is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

auto &r = resources_[i];
return selection_type{*this, r};
} else {
throw std::logic_error(“selected called before initialization”);
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems "selected" is intentional since it is repeated, and probably coming from the code.
I think we would want "selected before initialization" or "select called before initialization".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed


May throw `std::bad_alloc` or `std::logic_error`.

A call to `get_resources` may cause lazy initialization. Initialization may throw `std::bad_alloc`.
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably need to define "lazy initialization" for a backend (and its resources).

vossmjp and others added 8 commits February 28, 2025 15:07
Co-authored-by: Dan Hoeflinger <109972525+danhoeflinger@users.noreply.github.com>
Co-authored-by: Dan Hoeflinger <109972525+danhoeflinger@users.noreply.github.com>
Co-authored-by: Dan Hoeflinger <109972525+danhoeflinger@users.noreply.github.com>
Co-authored-by: Dan Hoeflinger <109972525+danhoeflinger@users.noreply.github.com>
Co-authored-by: Dan Hoeflinger <109972525+danhoeflinger@users.noreply.github.com>
Co-authored-by: Dan Hoeflinger <109972525+danhoeflinger@users.noreply.github.com>
@vossmjp vossmjp requested review from akukanov and rarutyun February 28, 2025 22:12
@vossmjp vossmjp marked this pull request as ready for review February 28, 2025 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants