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

[RFCs] Add Flow Graph try_put_and_wait RFC #1513

Open
wants to merge 38 commits into
base: master
Choose a base branch
from

Conversation

kboyarinov
Copy link
Contributor

Description

Add a comprehensive description of proposed changes

Fixes # - issue number(s) if exists

Type of change

Choose one or multiple, leave empty if none of the other choices apply

Add a respective label(s) to PR if you have permissions

  • bug fix - change that fixes an issue
  • new feature - change that adds functionality
  • tests - change in tests
  • infrastructure - change in infrastructure and CI
  • documentation - documentation update

Tests

  • added - required for new features and some bug fixes
  • not needed

Documentation

  • updated in # - add PR number
  • needs to be updated
  • not needed

Breaks backward compatibility

  • Yes
  • No
  • Unknown

Notify the following users

List users with @ to send notifications

Other information

vossmjp and others added 30 commits August 1, 2024 13:59
Co-authored-by: Aleksei Fedotov <[email protected]>
Co-authored-by: Aleksei Fedotov <[email protected]>
Co-authored-by: Aleksei Fedotov <[email protected]>
Co-authored-by: Aleksei Fedotov <[email protected]>
Co-authored-by: Aleksei Fedotov <[email protected]>
Co-authored-by: Aleksei Fedotov <[email protected]>
Co-authored-by: Aleksei Fedotov <[email protected]>
Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Aleksei Fedotov <[email protected]>
Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Alexandra <[email protected]>
Made suggested wording changes.

Co-authored-by: Alexandra <[email protected]>
Co-authored-by: Aleksei Fedotov <[email protected]>
@kboyarinov kboyarinov marked this pull request as draft September 13, 2024 09:35
Base automatically changed from dev/vossmjp/rfcs to master September 26, 2024 14:02
@kboyarinov kboyarinov marked this pull request as ready for review January 6, 2025 15:42
@vossmjp vossmjp added the RFC label Jan 9, 2025
Otherwise, if the concurrency limit of the node is reached, both message and the associated metainformations would be rejected and the predecessor that called the ``try_put_task``
is responsible on buffering both of them.

If the predecessor is not the buffering node, both message and the metainfo would be lost.
Copy link
Contributor

@vossmjp vossmjp Mar 26, 2025

Choose a reason for hiding this comment

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

You should say what "lost" means in this context. How does it affect waiting? Does a lost message/metainfo properly decrement the count?

Comment on lines +285 to +286
Since the metainformation is kept in the buffer together with the message itself, even if the computation is completed, the ``try_put_and_wait`` would stuck because of the reference
held by the buffer.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Since the metainformation is kept in the buffer together with the message itself, even if the computation is completed, the ``try_put_and_wait`` would stuck because of the reference
held by the buffer.
Since the metainformation is kept in the buffer together with the message itself, even if the message is consumed by a successor,
``try_put_and_wait`` will not complete because of the reference held by the buffer until the node is explicitly cleared.

Comment on lines +292 to +293
* The ``overwrite_node`` should be explicitly reset by calling ``node.reset()`` or the element with the stored metainfo should be overwritten with another element.
* The ``write_once_node`` should be explicitly reset by calling ``node.reset()`` since the item cannot be overwritten.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* The ``overwrite_node`` should be explicitly reset by calling ``node.reset()`` or the element with the stored metainfo should be overwritten with another element.
* The ``write_once_node`` should be explicitly reset by calling ``node.reset()`` since the item cannot be overwritten.
* The ``overwrite_node`` should be explicitly reset by calling ``node.clear()`` or the element with the stored metainfo should be overwritten with another element.
* The ``write_once_node`` should be explicitly reset by calling ``node.clear()`` since the item cannot be overwritten.


### Queueing ``join_node``

Each input port of the join_node should support the queue for both values and the associated metainformations. Once all of the input ports would contain the value, the values
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Each input port of the join_node should support the queue for both values and the associated metainformations. Once all of the input ports would contain the value, the values
Each input port of the join_node should support the queue for both values and the associated metainformations. Once all of the input ports contain the value, the values

Comment on lines +311 to +312
If the item with the metainformation is stored in the internal queue of one of the input ports, but items never received by other ports, the item and the metainformation would be kept in the
queue and block the corresponding ``try_put_and_wait`` call.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If the item with the metainformation is stored in the internal queue of one of the input ports, but items never received by other ports, the item and the metainformation would be kept in the
queue and block the corresponding ``try_put_and_wait`` call.
If an item with the metainformation is stored in the internal queue of one of the input ports, but items are never received by other ports, the item and the metainformation will be kept in the
queue and block the corresponding ``try_put_and_wait`` call.

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 need to add a clear function to join_node to allow explicit clearing of the internal queues.

* Multi-output nodes support should be described and implemented
* Feedback from the customers should be received
* More multithreaded tests should be implemented for the existing functionality
* The corresponding oneAPI specification update should be done
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a clear function to all buffering nodes?

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.

2 participants