Skip to content

Conversation

@avik-pal
Copy link
Collaborator

No description provided.

@avik-pal avik-pal force-pushed the ap/traced_call_v2 branch 3 times, most recently from a57d94f to 9549a6e Compare September 13, 2025 02:07
@avik-pal avik-pal requested a review from wsmoses September 13, 2025 02:39
@codecov
Copy link

codecov bot commented Sep 13, 2025

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 68.86%. Comparing base (8040a3e) to head (8a5ef1d).

Files with missing lines Patch % Lines
src/TracedRArray.jl 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1662      +/-   ##
==========================================
+ Coverage   68.82%   68.86%   +0.04%     
==========================================
  Files         103      103              
  Lines       11596    11599       +3     
==========================================
+ Hits         7981     7988       +7     
+ Misses       3615     3611       -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.

@avik-pal avik-pal changed the title feat: split out non-generator changes from #1642 feat: caching code in mapreduce style ops Sep 13, 2025
@avik-pal avik-pal requested a review from jumerckx September 13, 2025 15:30
end

(fn::BroadcastIterator)(args...) = Reactant.call_with_reactant(fn.f, (args...,))
(fn::BroadcastIterator)(args...) = fn.f((args...,))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Trying to understand what this is doing. Why isn't call_with_reactant required anymore, or why was it required before?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is meant to be only used instead broadcast which with force the call_via_reactant in elem_apply (it should have not been added in the first place)

@avik-pal avik-pal requested a review from jumerckx September 15, 2025 03:24
@jumerckx
Copy link
Collaborator

So this looks reasonable to me (though not sure if the CI failure is related to this pr?)

One major caveat is that the caching mechanism for Ops.call isn't correct if the function mutates a non-traced Julia object. e.g.:
for

function f(X, y)
	push!(X, y)
end
# X::Vector{TracedRArray}
# y::TracedRArray

because in this case tracing through f has a side-effect that isn't captured when simply inserting a func.call to the existing func.func.

Billy mentioned the need for an analysis that tells whether such a mutation occurs. I've looked into this a little bit but haven't yet found a good way to do this.

@avik-pal
Copy link
Collaborator Author

Ah I see, in that case, let's hold off on the caching part. I will separate out the non-caching fixes in a separate PR

* feat: better support for Base.Generators

* feat: use traced_call when unrolling iterators and generators

* fix: closure with call working

* fix: try removing nospecialize

* fix: use a looped version of any to avoid inference issues

* fix: dont overlay inside compiler call
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants