[WIP][SYSTEMDS-3907] tee ooc operator by j143 · Pull Request #2317 · apache/systemds

j143 · 2025-08-28T03:15:03Z

implement tee ooc operator

Done:

address review comment - moving TeeOp to DataOp

TODO:
2. presently only, meant for t(X) %*% X, need to see if this works in general.

j143

Hi @mboehm7 , could you please check this, I would be grateful for any review comments.

j143 · 2025-08-28T18:40:08Z

 		if (hop.getParent().size() > 1) {
 			for (Hop consumer : hop.getParent()) {
-				if (consumer instanceof TeeOp) {
+				if (consumer instanceof DataOp) {


this check only checking for DataOp.

j143 · 2025-08-28T18:41:31Z

 		// Create the new TeeOp with the original hop as input
-		TeeOp teeOp = new TeeOp(sharedInput);
+//		TeeOp teeOp = new TeeOp(sharedInput);
+		DataOp teeOp	= new DataOp("tee_out_" + sharedInput.getName(),


presently we are creating different variable names than the ones created automatically _mvar1 ... --> is this the correct approach?

no worries, these hop names are just for debugging - while generating instructions, we generate the _mvar, _fvar, _var accordingly.

j143 · 2025-08-28T18:41:55Z

+		DataOp teeOp	= new DataOp("tee_out_" + sharedInput.getName(),
+						sharedInput.getDataType(),
+						sharedInput.getValueType(),
+						Types.OpOpData.TRANSIENTWRITE,


Is it supposed to be TRANSIENTWRITE or TRANSIENTREAD?

j143 · 2025-08-28T18:42:50Z

-import org.apache.sysds.lops.LopsException;
-import org.apache.sysds.lops.Sql;


I'll remove *

yes exactly - let's try to avoid the wildcard imports (to avoid ambiguity).

codecov · 2025-08-28T18:49:54Z

Codecov Report

❌ Patch coverage is 88.94472% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.28%. Comparing base (8e9aaa5) to head (57890b2).
⚠️ Report is 16 commits behind head on main.

Files with missing lines	Patch %	Lines
...apache/sysds/hops/rewrite/RewriteInjectOOCTee.java	85.71%	5 Missing and 8 partials ⚠️
src/main/java/org/apache/sysds/hops/DataOp.java	90.24%	2 Missing and 2 partials ⚠️
...ds/runtime/instructions/ooc/TeeOOCInstruction.java	89.18%	4 Missing ⚠️
src/main/java/org/apache/sysds/lops/Tee.java	92.85%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2317      +/-   ##
============================================
+ Coverage     72.26%   72.28%   +0.01%     
- Complexity    46636    46690      +54     
============================================
  Files          1496     1499       +3     
  Lines        176862   177039     +177     
  Branches      34767    34794      +27     
============================================
+ Hits         127805   127965     +160     
- Misses        39407    39420      +13     
- Partials       9650     9654       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mboehm7

Thanks for making progress here @j143 - below you find my answers, additional comments, and a recommendation for a design change that might simplify things.

mboehm7 · 2025-08-29T08:40:06Z

-import org.apache.sysds.lops.LopsException;
-import org.apache.sysds.lops.Sql;


yes exactly - let's try to avoid the wildcard imports (to avoid ambiguity).

mboehm7 · 2025-08-29T08:40:59Z


 	private boolean _recompileRead = true;

+	private boolean _isTeeOp = false;


I would rather make it a op-type of DataOp like TWRITE.

mboehm7 · 2025-08-29T09:47:07Z

-		setLineNumbers(l);
-		setLops(l);
+//		setLineNumbers(l);
+//		setLops(l);


the line numbers and lops should also be set for the TeeOp

mboehm7 · 2025-08-29T09:50:01Z

 		// Create the new TeeOp with the original hop as input
-		TeeOp teeOp = new TeeOp(sharedInput);
+//		TeeOp teeOp = new TeeOp(sharedInput);
+		DataOp teeOp	= new DataOp("tee_out_" + sharedInput.getName(),


no worries, these hop names are just for debugging - while generating instructions, we generate the _mvar, _fvar, _var accordingly.

mboehm7 · 2025-08-29T09:50:38Z

+		boolean isTransposeMM = isTranposePattern(hop);
+		boolean isMatrix = hop.getDataType() == Types.DataType.MATRIX;
+
+		return isOOCEnabled && multipleConsumers && isNotAlreadyTee && isTransposeMM && isMatrix;


it's fine to currently only do it for transpose - it will not affect other things, and later we generalize the approach.

mboehm7 · 2025-08-29T09:52:15Z

+		for (Hop parent: hop.getParent()) {
+			String opString = parent.getOpString();
+			if (parent instanceof ReorgOp) {
+				if (opString.contains("r'") || opString.contains("transpose")) {


please always use the following utils: HopRewriteUtils.isReorgOp(parent, ReOrgOp.TRANS)

mboehm7 · 2025-08-29T09:52:29Z

+				}
+			}
+			else if (parent instanceof AggBinaryOp)
+				if (opString.contains("*") || opString.contains("ba+*")) {


same here as above.

mboehm7 · 2025-08-29T09:58:30Z

+		String[] out = outputs.split(Lop.OPERAND_DELIMITOR);
+		String output2 = outputs + "_copy";
+
+		// This method generates the instruction string: OOC°tee°input°output1°output2...


I would recommend not making tee an operation with two outputs but just one. (copying the input). After thinking a bit more about it, maybe the entire notion of a tee was not a good idea. Potentially, we could make the stream resettable, and the tee operator takes a normal stream, and outputs a resettable stream which can then be consumed by arbitrary many other operations (which also solves the problem of complex control flow interactions where the multiple parents are not immediately visible)

thanks @mboehm7 . Are you referring to the way, where we have lightweight marker for the stream so the multiple consumers could utilize it. Please let me raise a different PR with the option.

yes exactly - I'm thinking about something like setResettable() on the taskqueue, which means the blocks are never removed (and potentially subject to eviction) but different operations can get new iterables that always iterate the all blocks. The first operation would simply pull all blocks into memory, and the rmvar then drops all of them. If there is a single consumer we would not need to make it resettable, but can just stream them in.

j143 added 2 commits August 28, 2025 08:38

[SYSTEMDS-3907] OOC Tee operator

ae62b29

fix formatting

2b90ce0

github-project-automation Bot added this to SystemDS PR Queue Aug 28, 2025

github-project-automation Bot moved this to In Progress in SystemDS PR Queue Aug 28, 2025

remove TeeOp class and use DataOp

57890b2

j143 commented Aug 28, 2025

View reviewed changes

mboehm7 reviewed Aug 29, 2025

View reviewed changes

mboehm7 closed this in e51db27 Sep 22, 2025

github-project-automation Bot moved this from In Progress to Done in SystemDS PR Queue Sep 22, 2025

		import org.apache.sysds.lops.LopsException;
		import org.apache.sysds.lops.Sql;


		private boolean _recompileRead = true;

		private boolean _isTeeOp = false;

Uh oh!

Conversation

j143 commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

j143 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

j143 Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Aug 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mboehm7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

j143 commented Aug 28, 2025 •

edited

Loading

j143 Aug 28, 2025 •

edited

Loading

codecov Bot commented Aug 28, 2025 •

edited

Loading