Skip to content

Conversation

@yebai
Copy link
Member

@yebai yebai commented Jan 23, 2022

Customised Libtask.Instruction{observe} is no longer required after TuringLang/Libtask.jl#102

@codecov
Copy link

codecov bot commented Jan 23, 2022

Codecov Report

❗ No coverage uploaded for pull request base (releases@fe4a188). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             releases      #42   +/-   ##
===========================================
  Coverage            ?   55.46%           
===========================================
  Files               ?        5           
  Lines               ?      366           
  Branches            ?        0           
===========================================
  Hits                ?      203           
  Misses              ?      163           
  Partials            ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe4a188...e45867c. Read the comment docs.

@yebai
Copy link
Member Author

yebai commented Jan 23, 2022

@KDr2 Can you take a look at what's causing the following error? I can't seem to find an obvious issue.

https://github.com/TuringLang/AdvancedPS.jl/runs/4914462011?check_suite_focus=true#step:6:63

@KDr2
Copy link
Member

KDr2 commented Jan 23, 2022

@KDr2 Can you take a look at what's causing the following error? I can't seem to find an obvious issue.

https://github.com/TuringLang/AdvancedPS.jl/runs/4914462011?check_suite_focus=true#step:6:63

You mean the warning?

@yebai
Copy link
Member Author

yebai commented Jan 24, 2022

That's correct.

┌ Warning: Unknown IR code: 
│   st = IRTools.Inner.Statement(:(AdvancedPS.observe), Any, 6)
└ @ Libtask ~/.julia/packages/Libtask/0JzcB/src/tapedfunction.jl:251

@KDr2
Copy link
Member

KDr2 commented Jan 24, 2022

This line of IR IRTools.Inner.Statement(:(AdvancedPS.observe), Any, 6) is neither a function call nor a new operator. For these statements, I don't know how many kinds are there, all I have met are Type constant (as this one), and Type Constant works fine now. In order to find other unknown kinds, I added this warning. So it is fine to omit this message here.

@yebai
Copy link
Member Author

yebai commented Jan 24, 2022

@KDr2 Sorry for the confusion - the question is why we have statements like IRTools.Inner.Statement(:(AdvancedPS.observe), Any, 6)? Let's figure out where it is coming from, and fix it if possible.

@KDr2
Copy link
Member

KDr2 commented Jan 24, 2022

This is a piece of IRCode which shows the situation:

1: (%1)
  %22 = Base.identity(%1)
  %23 = Libtask.track!(%22, Libtask.box_args)
  %2 = Libtask.track!(%22, Main.Normal, 0, 1)
  %3 = Libtask.track!(%22, Main.rand, %2)
  %4 = Libtask.track!(%22, Base.setproperty!, %1, :a, %3)
  %5 = Libtask.track!(%22, Main.Bernoulli, 1)
  %6 = Libtask.track!(%22, Main.rand, %5)
  %7 = Libtask.track!(%22, Base.setproperty!, %1, :x, %6)
  %8 = Libtask.track!(%22, Main.Gamma, 2, 3)
  %9 = Libtask.track!(%22, Main.rand, %8)
  %10 = Libtask.track!(%22, Base.setproperty!, %1, :b, %9)
  %11 = AdvancedPS.observe   # <-------------------------------------- HERE
  %12 = Libtask.track!(%22, Main.:/, %6, 2)
  %13 = Libtask.track!(%22, Main.Bernoulli, %12)
  %14 = Libtask.track!(%22, %11, %13, 1) # <--------------------------- USED HERE
  %15 = Libtask.track!(%22, Main.Beta)
  %16 = Libtask.track!(%22, Main.rand, %15)
  %17 = Libtask.track!(%22, Base.setproperty!, %1, :c, %16)
  %18 = AdvancedPS.observe # <-------------------------------------- HERE
  %19 = %6 / 2
  %20 = Main.Bernoulli(%19)
  %21 = (%18)(%20, 0) # <--------------------------- USED HERE
  %24 = Base.identity(%21)
  return %24

It seems sometimes a = f(b) will be translated to

%1 = f
%a = (%1)(%b)

@yebai
Copy link
Member Author

yebai commented Jan 24, 2022

thanks, @KDr2 for looking into this. This is a bug. We need to track the statements below onto the tape too, otherwise, the results can be incorrect.

  %18 = AdvancedPS.observe # <-------------------------------------- HERE
  %19 = %6 / 2
  %20 = Main.Bernoulli(%19)
  %21 = (%18)(%20, 0) # <--------------------------- USED HERE
  %24 = Base.identity(%21)

@KDr2
Copy link
Member

KDr2 commented Jan 25, 2022

In the ctrl-flow PR, this kind of statement is dealt with and put on the tape:

https://github.com/TuringLang/Libtask.jl/pull/107/files#diff-32dbddadf7a543bf11f421085d2833eaed85ae8599084d725af862eccd8dae2fR253

@KDr2
Copy link
Member

KDr2 commented Jan 25, 2022

And before that PR, because we need to run the IR to track all the calls, as it runs, all this kind of Statements are evaluated and are used as constant input arguments in instructions, so it works as well.

@KDr2
Copy link
Member

KDr2 commented Jan 25, 2022

I re-run the code and print the IR code, it is correct now:

ir = 1: (%1)
  %22 = Base.identity(%1)
  %23 = Libtask.track!(%22, Libtask.box_args)
  %2 = Libtask.track!(%22, Main.Normal, 0, 1)
  %3 = Libtask.track!(%22, Main.rand, %2)
  %4 = Libtask.track!(%22, Base.setproperty!, %1, :a, %3)
  %5 = Libtask.track!(%22, Main.Bernoulli, 1)
  %6 = Libtask.track!(%22, Main.rand, %5)
  %7 = Libtask.track!(%22, Base.setproperty!, %1, :x, %6)
  %8 = Libtask.track!(%22, Main.Gamma, 2, 3)
  %9 = Libtask.track!(%22, Main.rand, %8)
  %10 = Libtask.track!(%22, Base.setproperty!, %1, :b, %9)
  %11 = AdvancedPS.observe
  %12 = Libtask.track!(%22, Main.:/, %6, 2)
  %13 = Libtask.track!(%22, Main.Bernoulli, %12)
  %14 = Libtask.track!(%22, %11, %13, 1)
  %15 = Libtask.track!(%22, Main.Beta)
  %16 = Libtask.track!(%22, Main.rand, %15)
  %17 = Libtask.track!(%22, Base.setproperty!, %1, :c, %16)
  %18 = AdvancedPS.observe
  %19 = Libtask.track!(%22, Main.:/, %6, 2)
  %20 = Libtask.track!(%22, Main.Bernoulli, %19)
  %21 = Libtask.track!(%22, %18, %20, 0)
  %24 = Libtask.track!(%22, Base.identity, %21)
  return %22

I did no changes except printing the IR...

@yebai
Copy link
Member Author

yebai commented Jan 25, 2022

@KDr2 thanks, can you fix the warning messages for the %18 = AdvancedPS.observe statements in Libtask?

EDIT: shouldn't the following return %24 instead of %22?

%24 = Libtask.track!(%22, Base.identity, %21)
  return %22  # should this be %24??

@yebai yebai merged commit 7891a3a into releases Jan 25, 2022
@delete-merged-branch delete-merged-branch bot deleted the hg/update-libtask-api branch January 25, 2022 16:37
@KDr2
Copy link
Member

KDr2 commented Jan 26, 2022

I find the reason why the last few lines were not tracked: I put the print(ir) statement inside the loop, after the @warn st, so when they hadn't been rewritten when the ir was printed (we just reached line %18). In the later debugging, I printed the ir after the loop finished.


%24 = Libtask.track!(%22, Base.identity, %21)
  return %22  # should this be %24??

Here we put the real return value onto the last instruction (as its output), and return the tape(%22).
When we run the tapedfunction, we see tape[end].output as its return value.


The GlobalRef issue is fixed by TuringLang/Libtask.jl#112

@yebai
Copy link
Member Author

yebai commented Jan 26, 2022

Thanks @KDr2!

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.

3 participants