[Merged by Bors] - Hotfixes after #309 #351

torfjelde · 2021-12-14T15:50:59Z

#309 was merged a bit too soon, for example bors was never run on the final version due to some issues. Before we make a release, we should make sure that it all works properly.

src/submodel_macro.jl

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

…cPPL.jl into tor/submodels-hotfixes

src/submodel_macro.jl

torfjelde · 2021-12-14T17:09:11Z

bors try

torfjelde · 2021-12-14T17:14:09Z

One thing is leaving me very confused. If I try to run the doctests locally, it'll complain because

DynamicPPL.jl/src/submodel_macro.jl

Line 166 in ce0a3e8

ERROR: LoadError: cannot automatically prefix with no left-hand side

results in LoadError: LoadError: rather than just LoadError. Aaaand I have no idea why 🤷

torfjelde · 2021-12-14T17:59:49Z

Is there something wrong with bors? It doesn't seem to want to comment despite all test-suites (except nightly) passing.

torfjelde · 2021-12-14T18:00:23Z

@devmotion @phipsgabler @yebai Could one of you have a look here? I think it's pretty "simple" given what we have in master.

yebai · 2021-12-14T18:11:26Z

src/submodel_macro.jl

        quote
-            $(esc(:__varinfo__)) = last(
-                $(DynamicPPL._evaluate!!)($(esc(expr)), $(esc(:__varinfo__)), $(ctx))
+            $retval, $(esc(:__varinfo__)) = $(DynamicPPL._evaluate!!)(


I like returning both the retval and varinfo. I was talking with Philip that we should consider enforcing this more systematically. Roughly speaking, we can consider adopting the notation that retval refers to generated quantities and varinfo refers to model parameters. This would help clarify the submodel notation as well, i.e.

x = SubModel will extract retval and assign it to LHS

x ~ SubModel will extract varinfo and assign it to LHS

Following this view, we no longer need to concern ourselves what is the semantics if the returned value retval of SubModel is deterministic in case 2.

I'm a bit confused. In both those cases we still have to concern ourselves with the return-value, no? Essentially the difference between = and ~ will be whether we have $retval or varinfo below, right?

Anyways, you're happy with this right? You're just talking about potentially also supporting ~ later?

yes, happy with this PR.

I think retval and varinfo provides a clear semantical distinction between = and ~. If we always return both, (e.g. we can return (, varinfo) in case retval is empty), we can extract either retval or varinfo for = or ~.

torfjelde · 2021-12-14T18:32:16Z

bors r+

#309 was merged a bit too soon, for example `bors` was never run on the final version due to some issues. Before we make a release, we should make sure that it all works properly.

devmotion · 2021-12-14T19:07:37Z

Is there something wrong with bors? It doesn't seem to want to comment despite all test-suites (except nightly) passing.

You broke it (or rather the last PR that was merged) 😛 It waits until all tests listed in bors.toml are finished (failed or succeeded) but since there are no tests of the form test (1.%...) it waits forever. You commented out the tests on Julia 1.3 (

DynamicPPL.jl/.github/workflows/CI.yml

Line 19 in 1744ba7

# - '1.3' # minimum supported version

) which these patterns refer to.

devmotion · 2021-12-14T19:11:44Z

results in LoadError: LoadError: rather than just LoadError. Aaaand I have no idea why shrug

Nested LoadErrors are a thing (e.g. JuliaLang/julia#38725) and I guess occur e.g. if modules are nested.

torfjelde · 2021-12-14T19:48:43Z

You broke it (or rather the last PR that was merged) stuck_out_tongue It waits until all tests listed in bors.toml are finished (failed or succeeded) but since there are no tests of the form test (1.%...) it waits forever.

As I said, I really didn't want the PR to get merged when it did 😅 But sorry! That was not the intended effect.

Nested LoadErrors are a thing (e.g. JuliaLang/julia#38725) and I guess occur e.g. if modules are nested.

I get that nested LoadError can occurr, but if I copy-paste the exact thing from the docstring into my REPL locally I get an additional LoadError: 🤷 It's the fact that it's different from my REPL and the docstring that confuses me.

bors · 2021-12-14T19:49:47Z

Canceled.

torfjelde · 2021-12-14T19:53:19Z

Any final things before we merge this @devmotion ?

devmotion · 2021-12-14T19:54:13Z

It's the fact that it's different from my REPL and the docstring that confuses

I think Documenter creates some modules internally, I think this explains differences between running the examples in the REPL and running the doc tests.

devmotion · 2021-12-14T19:56:00Z

Any final things before we merge this @devmotion ?

No, but it's also a bit difficult to see if e.g. anything else is missing 😄 I'm happy if bors is 🤖

torfjelde · 2021-12-14T20:04:26Z

It's the fact that it's different from my REPL and the docstring that confuses

I think Documenter creates some modules internally, I think this explains differences between running the examples in the REPL and running the doc tests.

That was my immediate thought as well, but I can't properly understand exactly what goes wrong, only that I guess this could be the cause.

But I'll leave the version that works with Documenter.jl 👍

No, but it's also a bit difficult to see if e.g. anything else is missing smile I'm happy if bors is robot

Yeah I know, def non-ideal with such a big change 😕

But I'll bors it then 👍

torfjelde · 2021-12-14T20:04:31Z

bors r+

#309 was merged a bit too soon, for example `bors` was never run on the final version due to some issues. Before we make a release, we should make sure that it all works properly.

bors · 2021-12-14T20:14:58Z

Build failed:

test (1.3, ubuntu-latest, x64, 1)

torfjelde · 2021-12-14T20:49:11Z

Sooo the behavior of LoadError seems to vary between Julia versions...

devmotion · 2021-12-14T20:49:32Z

It seems we have to add a doctestfilter (

DynamicPPL.jl/test/runtests.jl

Line 62 in 1744ba7

doctestfilters = [

and possibly in docs/make.jl) for LoadError: LoadError: with Julia 1.3?

docs/make.jl

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

torfjelde · 2021-12-14T21:48:50Z

bors r+

#309 was merged a bit too soon, for example `bors` was never run on the final version due to some issues. Before we make a release, we should make sure that it all works properly.

bors · 2021-12-14T21:58:22Z

Build failed:

test (1.3, ubuntu-latest, x64, 1)

devmotion · 2021-12-14T22:05:14Z

docs/make.jl

@@ -18,6 +18,9 @@ makedocs(;
        r"(Array{.+,\s?1}|Vector{.+})",
        # Older versions will show "Array{...,2}" instead of "Matrix{...}".
        r"(Array{.+,\s?2}|Matrix{.+})",
+        # Errors from macros sometimes result in `LoadError: LoadError:`
+        # rather than `LoadError:`, depending on Julia version.
+        r"ERROR: LoadError: (LoadError:\s)?+",


Maybe just use

Suggested change

r"ERROR: LoadError: (LoadError:\s)?+",

r"ERROR: (LoadError:\s)+",

? And it seems you forgot to update test/runtests.jl?

Maybe just use

Is this correct though? Don't we want to match at least one LoadError?

And it seems you forgot to update test/runtests.jl?

Ah thanks!

Don't we want to match at least one LoadError?

+ matches one or more occurrences so it doesn't match only "ERROR: ":

julia> regex = r"ERROR: (LoadError:\s)+"; julia> match(regex, "ERROR: ") === nothing true julia> match(regex, "ERROR: LoadError: ") RegexMatch("ERROR: LoadError: ", 1="LoadError: ") julia> match(regex, "ERROR: LoadError: LoadError: ") RegexMatch("ERROR: LoadError: LoadError: ", 1="LoadError: ")

Haha, yeah I knew that, hence why I used ? on the second one. Now you might ask "then why did you just say what you just did?", to which I don't have a good answer 🙃 Thanks man!

EDIT: Oh wait, now I remember why I did it! If you do (LoadError:\s)+ you'll end up comparing the output to Error: , no? While we want to compare it to Error: LoadError: , i.e. all the redundant LoadError: removed?|

EDIT 2: Nope:) I guess it removes it from both the expected output and the actual output then.

The doctest filters will be applied to both the expected output and the actual output and then the matches will be removed. The remaining string is checked for equality. So the LoadError: will be removed from the expected output as well.

https://juliadocs.github.io/Documenter.jl/stable/man/doctests/#Filtering-Doctests

That was my original understanding when I wrote it, but I think I must have missed some whitespace so it didn't match but instead complained that "ERROR: ...ddin't matchERROR: LoadError: ...`, i.e. the display seemed to indicate it was only removed from the expected output which confused me.

What I've learned tonight: I should not let myself be so easily swayed by displayed information:)

bors · 2021-12-14T22:06:18Z

try

Timed out.

torfjelde · 2021-12-14T22:26:46Z

bors r+

#309 was merged a bit too soon, for example `bors` was never run on the final version due to some issues. Before we make a release, we should make sure that it all works properly.

bors · 2021-12-14T22:36:30Z

Build failed:

test (1.3, ubuntu-latest, x64, 1)

torfjelde · 2021-12-14T22:43:44Z

bors r+

#309 was merged a bit too soon, for example `bors` was never run on the final version due to some issues. Before we make a release, we should make sure that it all works properly.

bors · 2021-12-14T22:57:13Z

Pull request successfully merged into master.

Build succeeded:

attempt at fixing error doctest

ecf1e5d

github-actions bot reviewed Dec 14, 2021

View reviewed changes

src/submodel_macro.jl Outdated Show resolved Hide resolved

torfjelde and others added 3 commits December 14, 2021 15:52

Update src/submodel_macro.jl

917334b

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

capture return-value in submodel

29d692a

Merge branch 'tor/submodels-hotfixes' of github.com:TuringLang/Dynami…

73962ad

…cPPL.jl into tor/submodels-hotfixes

github-actions bot reviewed Dec 14, 2021

View reviewed changes

src/submodel_macro.jl Outdated Show resolved Hide resolved

torfjelde added 2 commits December 14, 2021 16:05

formatting and added test for return-value and submodel

9ee336b

fixed typo

ce0a3e8

bors bot added a commit that referenced this pull request Dec 14, 2021

Try #351:

44cdf68

yebai approved these changes Dec 14, 2021

View reviewed changes

bors bot pushed a commit that referenced this pull request Dec 14, 2021

Hotfixes after #309 (#351)

337a5e3

#309 was merged a bit too soon, for example `bors` was never run on the final version due to some issues. Before we make a release, we should make sure that it all works properly.

fix CI so as to not break poor bors

59303d7

bors bot pushed a commit that referenced this pull request Dec 14, 2021

Hotfixes after #309 (#351)

b60bd84

#309 was merged a bit too soon, for example `bors` was never run on the final version due to some issues. Before we make a release, we should make sure that it all works properly.

added docfilter for nested LoadError

274aebb

github-actions bot reviewed Dec 14, 2021

View reviewed changes

docs/make.jl Outdated Show resolved Hide resolved

Update docs/make.jl

ef0faa5

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

bors bot pushed a commit that referenced this pull request Dec 14, 2021

Hotfixes after #309 (#351)

2dfa59b

#309 was merged a bit too soon, for example `bors` was never run on the final version due to some issues. Before we make a release, we should make sure that it all works properly.

devmotion reviewed Dec 14, 2021

View reviewed changes

torfjelde added 3 commits December 14, 2021 22:09

fixed doctestfilter

9ecb939

added LoadError doctest filter to tests

c4e91a0

improved the LoadError filter

9b32797

bors bot pushed a commit that referenced this pull request Dec 14, 2021

Hotfixes after #309 (#351)

2a0258c

#309 was merged a bit too soon, for example `bors` was never run on the final version due to some issues. Before we make a release, we should make sure that it all works properly.

updated turing tests compat bound

ad049c2

bors bot pushed a commit that referenced this pull request Dec 14, 2021

Hotfixes after #309 (#351)

0bb2504

#309 was merged a bit too soon, for example `bors` was never run on the final version due to some issues. Before we make a release, we should make sure that it all works properly.

bors bot changed the title ~~Hotfixes after #309~~ [Merged by Bors] - Hotfixes after #309 Dec 14, 2021

bors bot closed this Dec 14, 2021

bors bot deleted the tor/submodels-hotfixes branch December 14, 2021 22:57

yebai mentioned this pull request Dec 16, 2021

[WIP] VarInfo changes #188

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Hotfixes after #309 #351

[Merged by Bors] - Hotfixes after #309 #351

torfjelde commented Dec 14, 2021

torfjelde commented Dec 14, 2021

torfjelde commented Dec 14, 2021

torfjelde commented Dec 14, 2021

torfjelde commented Dec 14, 2021

yebai Dec 14, 2021 •

edited

Loading

torfjelde Dec 14, 2021

yebai Dec 14, 2021 •

edited

Loading

torfjelde commented Dec 14, 2021

devmotion commented Dec 14, 2021

devmotion commented Dec 14, 2021

torfjelde commented Dec 14, 2021 •

edited

Loading

bors bot commented Dec 14, 2021

torfjelde commented Dec 14, 2021

devmotion commented Dec 14, 2021

devmotion commented Dec 14, 2021

torfjelde commented Dec 14, 2021

torfjelde commented Dec 14, 2021

bors bot commented Dec 14, 2021

torfjelde commented Dec 14, 2021

devmotion commented Dec 14, 2021

torfjelde commented Dec 14, 2021

bors bot commented Dec 14, 2021

devmotion Dec 14, 2021

torfjelde Dec 14, 2021

devmotion Dec 14, 2021

torfjelde Dec 14, 2021 •

edited

Loading

devmotion Dec 14, 2021

devmotion Dec 14, 2021

torfjelde Dec 14, 2021

bors bot commented Dec 14, 2021

torfjelde commented Dec 14, 2021

bors bot commented Dec 14, 2021

torfjelde commented Dec 14, 2021

bors bot commented Dec 14, 2021

	r"ERROR: LoadError: (LoadError:\s)?+",
	r"ERROR: (LoadError:\s)+",

[Merged by Bors] - Hotfixes after #309 #351

[Merged by Bors] - Hotfixes after #309 #351

Conversation

torfjelde commented Dec 14, 2021

torfjelde commented Dec 14, 2021

torfjelde commented Dec 14, 2021

torfjelde commented Dec 14, 2021

torfjelde commented Dec 14, 2021

yebai Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

torfjelde Dec 14, 2021

Choose a reason for hiding this comment

yebai Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

torfjelde commented Dec 14, 2021

devmotion commented Dec 14, 2021

devmotion commented Dec 14, 2021

torfjelde commented Dec 14, 2021 • edited Loading

bors bot commented Dec 14, 2021

torfjelde commented Dec 14, 2021

devmotion commented Dec 14, 2021

devmotion commented Dec 14, 2021

torfjelde commented Dec 14, 2021

torfjelde commented Dec 14, 2021

bors bot commented Dec 14, 2021

torfjelde commented Dec 14, 2021

devmotion commented Dec 14, 2021

torfjelde commented Dec 14, 2021

bors bot commented Dec 14, 2021

devmotion Dec 14, 2021

Choose a reason for hiding this comment

torfjelde Dec 14, 2021

Choose a reason for hiding this comment

devmotion Dec 14, 2021

Choose a reason for hiding this comment

torfjelde Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

devmotion Dec 14, 2021

Choose a reason for hiding this comment

devmotion Dec 14, 2021

Choose a reason for hiding this comment

torfjelde Dec 14, 2021

Choose a reason for hiding this comment

bors bot commented Dec 14, 2021

try

torfjelde commented Dec 14, 2021

bors bot commented Dec 14, 2021

torfjelde commented Dec 14, 2021

bors bot commented Dec 14, 2021

yebai Dec 14, 2021 •

edited

Loading

yebai Dec 14, 2021 •

edited

Loading

torfjelde commented Dec 14, 2021 •

edited

Loading

torfjelde Dec 14, 2021 •

edited

Loading