compiler: Revamp fuse-task optoption#2975
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2975 +/- ##
==========================================
+ Coverage 83.54% 83.56% +0.02%
==========================================
Files 257 257
Lines 53591 53779 +188
Branches 4585 4611 +26
==========================================
+ Hits 44770 44943 +173
- Misses 8028 8037 +9
- Partials 793 799 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| # Fusion | ||
| o['fuse-tasks'] = oo.pop('fuse-tasks', False) | ||
| # Tasking | ||
| o['npthreads'] = oo.pop('npthreads', None) |
There was a problem hiding this comment.
nitpicking: might wanna throw a warning, or oo.pop('npthreads', oo.pop('fuse-tasks', None) for "safety"
There was a problem hiding this comment.
Is there any documentation that needs updating to reflect this change?
| return None | ||
|
|
||
| gid, = gids | ||
| return gid |
| # Fusion | ||
| o['fuse-tasks'] = oo.pop('fuse-tasks', False) | ||
| # Tasking | ||
| o['npthreads'] = oo.pop('npthreads', None) |
There was a problem hiding this comment.
Is there any documentation that needs updating to reflect this change?
| optype, = {type(op) for spot in self.sync_spots | ||
| for op in spot.sync_ops} |
There was a problem hiding this comment.
This should probably have some kind of error handling or at least an assert for the case there are mixed types here (and the set is not length-1), otherwise it's potentially going to lead to cryptic errors or even silent errors if the ValueError gets ingested by a try-except
| self.dim = dim | ||
| self.size = size | ||
| self.origin = origin | ||
| self.gid = gid |
There was a problem hiding this comment.
adding a docstring, but basically group id (a la Linux)
| body=Conditional(task.condition.condition, | ||
| task.spot.body)) | ||
| for task in tasks] | ||
| insertions[tasks[-1].anchor].append(SyncSpotRegion(spots)) |
There was a problem hiding this comment.
Nitpick: pulling out tasks[-1].anchor and assigning a descriptive variable name would improve readability here
There was a problem hiding this comment.
this is changed already, I knew I may still have had to tweak a couple of things, including this one
| return iet, [efunc] | ||
| layers = {infer_layer(i.function) for i in sync_ops} | ||
| if len(layers) != 1: | ||
| raise CompilationError("Unsupported streaming case") |
There was a problem hiding this comment.
Nitpick: worth adding why to the error?
There was a problem hiding this comment.
those 4 lines also appear in another place, so I'll move them into a utility function and improve the error message as per your suggestion
5f25d96 to
3afe7c7
Compare
3 participants
I'm also renaming it into what it should always have been in the first place --
'npthreads'. This is an undocumented option anyway and no-one is using it, so I think it's perfectly safeMore tests in PRO