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

Fix None output node bug in JITLinker.create_jitable_thunk #1205

Merged
merged 1 commit into from Sep 23, 2022

Conversation

ricardoV94
Copy link
Contributor

  • Also fixes bug in JITCompiler when first output of inner fgraph is an input variable, as can happen in some specific functions with updates

Closes #1195

@ricardoV94 ricardoV94 force-pushed the test_backend_update branch 2 times, most recently from d456376 to 0e3b78e Compare September 22, 2022 09:50
rlouf
rlouf previously approved these changes Sep 22, 2022
@codecov
Copy link

codecov bot commented Sep 22, 2022

Codecov Report

Merging #1205 (0a34abd) into main (5d97ffa) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1205   +/-   ##
=======================================
  Coverage   79.14%   79.14%           
=======================================
  Files         173      173           
  Lines       48485    48486    +1     
  Branches    10966    10966           
=======================================
+ Hits        38375    38376    +1     
  Misses       7607     7607           
  Partials     2503     2503           
Impacted Files Coverage Δ
aesara/link/utils.py 61.12% <ø> (ø)
aesara/link/basic.py 86.49% <100.00%> (ø)
aesara/sandbox/rng_mrg.py 84.14% <100.00%> (+0.03%) ⬆️

Comment on lines +644 to +645
# This is a bit hackish, but we only return one of the output nodes
output_nodes = [o.owner for o in self.fgraph.outputs if o.owner is not None][:1]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wasn't changed by this PR, but I wonder does the hack actually work? There's a whole bunch of logic downstream concerning garbage collection and whatnot that is supposed to rely on this. Too dense for me to take a look though :D

Copy link
Member

Choose a reason for hiding this comment

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

This wasn't changed by this PR, but I wonder does the hack actually work? There's a whole bunch of logic downstream concerning garbage collection and whatnot that is supposed to rely on this. Too dense for me to take a look though :D

It must be working; how else have we been using Numba/JAX to evaluate graphs?

The hackish part is that we were and still are (to some extent) using the old Function/VM framework to do something that it wasn't designed to do. For example, we don't have a thunk for each node in the graph, like we normally would.

To be clear, shared variable and updates support for the JIT backends has always been incomplete, and this falls squarely into the latter.

* Also fixes bug in JITCompiler when first output of inner fgraph is an input variable, as can happen in some specific functions with updates
@brandonwillard brandonwillard merged commit 3aff3d2 into aesara-devs:main Sep 23, 2022
@brandonwillard brandonwillard changed the title Add tests for shared updates in JAX and NUMBA backends Fix None output node bug in JITLinker.create_jitable_thunk Sep 23, 2022
@brandonwillard brandonwillard added bug Something isn't working Numba Involves Numba transpilation labels Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Numba Involves Numba transpilation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in JITLinker when first output of inner FunctionGraph is an input variable
3 participants