compiler: Revamp fuse-task optoption#2975
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2975 +/- ##
==========================================
+ Coverage 83.53% 83.58% +0.05%
==========================================
Files 257 257
Lines 53601 53783 +182
Branches 4586 4608 +22
==========================================
+ Hits 44775 44955 +180
Misses 8032 8032
- Partials 794 796 +2
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:
|
| return None | ||
|
|
||
| gid, = gids | ||
| return gid |
| 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
| 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
9c40646 to
741e1b1
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