Version the graph examples #84

nirvdrum · 2024-03-15T22:56:02Z

This PR restores the script for generating examples and modernizes it.

We were unable to run the Ruby examples with GraalVM 21.2.0 because Graal was missing a necessary option. When those results were generated manually they were done with a patched version of Graal. Subsequent versions of Graal have the option enabled so this script can now process the Ruby examples.

To help keep storage size down, I've switched all the top-level examples to symlinks to a GraalVM-versioned set of files. Now that the Ruby example graphs are generated automatically, I've compressed them just like the Java example graphs.

I think is in keeping with what @chrisseaton had in mind in #37. I haven't yet added the ability to directly compare graphs from different GraalVM versions, although that can be done approximately with the seafoam describe command.

* Run the Ruby examples on GraalVM distributions that support it. * Added GraalVM 22.3.1 to the list of GraalVM distributions used for graph generation. * Restructure the way we run commands to enable code reuse.

rwstauner

Seems good to me.
A few minor nits but fine as is.

tools/generate-examples.rb

…erence example graphs.

…rmat. GraalVM used to ship as a JVM into which one could install multiple languages with a tool named `gu`. The `gu` utility was removed and languages are now avaliable as Maven artifacts. While that's considerably more convenient for applications embedding Truffle languages, it complicates using each language's launcher. Consequently, we now need to install separate distributions for each language in order to run the examples for that language.

rwstauner · 2024-03-19T21:56:03Z

tools/generate-examples.rb

+      FileUtils.cp("fib-java.bgv.gz", "#{__dir__}/../examples/#{graalvm.name}")
+
+      if reference_graalvm?(graalvm)
+        FileUtils.ln_sf("#{__dir__}/../examples/#{graalvm.name}/fib-java.bgv.gz", "#{__dir__}/../examples")


This is creating symlinks with absolute paths

Does this work?

Suggested change

FileUtils.ln_sf("#{__dir__}/../examples/#{graalvm.name}/fib-java.bgv.gz", "#{__dir__}/../examples")

FileUtils.ln_sf("#{graalvm.name}/fib-java.bgv.gz", "#{__dir__}/../examples")

Argh. I thought I fixed all of those. I'll take another look. Thanks!

tools/generate-examples.rb

rwstauner · 2024-03-19T22:09:24Z

tools/generate-examples.rb

+        FileUtils.ln_sf("#{__dir__}/../examples/#{graalvm.name}/fib-js.bgv.gz", "#{__dir__}/../examples/")
+        FileUtils.ln_sf(


rwstauner · 2024-03-19T22:09:33Z

tools/generate-examples.rb

+        FileUtils.ln_sf("#{__dir__}/../examples/#{graalvm.name}/fib-ruby.bgv.gz", "#{__dir__}/../examples")
+        FileUtils.ln_sf(


rwstauner · 2024-03-19T22:12:33Z

tools/generate-examples.rb

+      raise unless bgv.size == 1
+      raise if bgv_ast.size > 1


Is there a good error message we could include here? Like a pointer as to what file to go look at if these sizes aren't right? Or would it be more likely to be a failure in a command?

I really don't know. It's a sanity check so the code that follows makes sense. I can't think of a reason why there would ever be more than one AST file unless Graal changes something unexpectedly. If that's the case, all we can really do is look at the error and reevaluate what assumption no longer holds.

tools/generate-examples.rb

rwstauner

As long as we fix the symlinks the rest of the comments aren't blockers, feel free to take them or leave them.

rwstauner · 2024-03-19T22:20:46Z

tools/generate-examples.rb

+      raise unless bgv.size == 1
+      raise if bgv_ast.size > 1


same, is there any hint worth providing about what to look into if this was an issue?

rwstauner

The tests pass for me locally.
I tried the tool and it updates all the examples, but then I get a single test failure:

rspec spec
........................................................F........................................................................................

Failures:

  1) Seafoam::Commands#props prints properties for a file
     Failure/Error: expect(@out.string.gsub("\n\n", "\n")).to(eq("{\n  \"vm.uuid\": \"21734\"\n}\n"))
     
       expected: "{\n  \"vm.uuid\": \"21734\"\n}\n"
            got: "{\n  \"vm.uuid\": \"64368\"\n}\n"
     
       (compared using ==)
     
       Diff:
       @@ -1,4 +1,4 @@
        {
       -  "vm.uuid": "21734"
       +  "vm.uuid": "64368"
        }
       
     # ./spec/seafoam/command_spec.rb:253:in `block (3 levels) in <top (required)>'

Finished in 1.1 seconds (files took 0.10114 seconds to load)
145 examples, 1 failure

Failed examples:

rspec ./spec/seafoam/command_spec.rb:251 # Seafoam::Commands#props prints properties for a file

rake aborted!
Command failed with status (1): [rspec spec...]
/Users/rwstauner/src/github.com/Shopify/seafoam/Rakefile:8:in `block in <top (required)>'
/Users/rwstauner/ruby/rbenv/versions/3.3.0/bin/bundle:25:in `load'
/Users/rwstauner/ruby/rbenv/versions/3.3.0/bin/bundle:25:in `<main>'
Tasks: TOP => specs
(See full trace by running task with --trace)

Any idea why that is or what it means? Is it worth fixing

seafoam/spec/seafoam/command_spec.rb

Line 253 in 741310d

    
           expect(@out.string.gsub("\n\n", "\n")).to(eq("{\n  \"vm.uuid\": \"21734\"\n}\n"))

somehow? Do we care what the value is, or could we change it to just match digits?

expect(@out.string.gsub("\n\n", "\n")).to(match(/{\n  "vm\.uuid": "\d+"\n}/))

nirvdrum · 2024-03-20T14:18:28Z

The UUID is unique for each graph. If you regenerate the examples, you'll get a new graph. While it's inconvenient that the one spec would fail, the expectation is that you'll go verify the UUID is correct by other means and ensure the test is checking for the correct value. If we made it a regex pattern it could result in the parser breaking without anyone noticing.

rwstauner · 2024-03-20T14:31:10Z

I did notice that in 2 different runs I got different values, but I thought if it was always different I would have seen you update it... which I didn't. I do see it has changed a few times in the history.

What does the value mean? Or what is it tied to?

Now I realize that when I run the script I have updates to examples/graalvm-ce-java11-21.2.0 but this PR doesn't change those. If I just restore that dir the spec passes, which explains why you didn't have to update the value in your PR.
Just to check my sanity: does the script update your examples/graalvm-ce-java11-21.2.0 dir? Did you just not include them because they didn't need to change (to keep the diff down)?

…he reference examples to use GraalVM 23.1.2.

nirvdrum · 2024-03-20T16:35:08Z

Now I realize that when I run the script I have updates to examples/graalvm-ce-java11-21.2.0 but this PR doesn't change those. If I just restore that dir the spec passes, which explains why you didn't have to update the value in your PR. Just to check my sanity: does the script update your examples/graalvm-ce-java11-21.2.0 dir? Did you just not include them because they didn't need to change (to keep the diff down)?

I didn't include them because there was no real need to regenerate them. Moreover, we can't fully regenerate them without patching Graal since GraalVM 21.1.0 did not support the --engine.InlineOnly option. Those graphs were generated manually with a patched version of Graal.

I suppose I could just not regenerate the graphs if the directory exists, but if we add new examples or change any of the engine options, we could then miss updates. In either case the person running the tool needs to be aware of what they're pushing. Given the node IDs change, this operation will never be idempotent, so it's incumbent on the person running the script to only commit meaningful changes.

rwstauner · 2024-03-20T16:47:07Z

Thanks, that makes sense to me 👍

* Added optional logging of commands being run. * Fixed deprecated usage of Graal options. * Cleaned up the reference to the examples directory.

nirvdrum added 6 commits March 14, 2024 16:24

Restore the script for generating examples.

f0fa046

Ignore GraalVM distributions used for generating examples.

af92e36

Ignore Graal graph dumps.

2401474

Overhaul the example generator.

cdbe2da

* Run the Ruby examples on GraalVM distributions that support it. * Added GraalVM 22.3.1 to the list of GraalVM distributions used for graph generation. * Restructure the way we run commands to enable code reuse.

Compress the Ruby example graphs.

f2a9bbc

Use git symlinks rather than copy the example files.

45dc514

nirvdrum requested a review from rwstauner March 15, 2024 22:56

rwstauner approved these changes Mar 15, 2024

View reviewed changes

tools/generate-examples.rb Outdated Show resolved Hide resolved

tools/generate-examples.rb Show resolved Hide resolved

tools/generate-examples.rb Outdated Show resolved Hide resolved

nirvdrum added 2 commits March 18, 2024 00:03

Process the example generation script with Rubocop.

4ef766b

Make it clearer which GraalVM distribution should be used for the ref…

ff10661

…erence example graphs.

nirvdrum force-pushed the version-examples branch from 01243ba to ff10661 Compare March 18, 2024 04:23

nirvdrum changed the title ~~Version examples~~ Version the graph examples Mar 19, 2024

nirvdrum force-pushed the version-examples branch 2 times, most recently from 5d082a8 to 94e5dda Compare March 19, 2024 21:31

nirvdrum marked this pull request as ready for review March 19, 2024 21:36

rwstauner requested changes Mar 19, 2024

View reviewed changes

rwstauner reviewed Mar 19, 2024

View reviewed changes

nirvdrum force-pushed the version-examples branch from 94e5dda to 741310d Compare March 19, 2024 22:40

rwstauner approved these changes Mar 19, 2024

View reviewed changes

Add graph examples from a few different GraalVM releases and update t…

2bae534

…he reference examples to use GraalVM 23.1.2.

General cleanup of the example generator script.

14794a9

* Added optional logging of commands being run. * Fixed deprecated usage of Graal options. * Cleaned up the reference to the examples directory.

nirvdrum force-pushed the version-examples branch from 741310d to 14794a9 Compare March 20, 2024 17:52

nirvdrum merged commit e1d0afa into main Mar 20, 2024
17 checks passed

nirvdrum deleted the version-examples branch March 20, 2024 17:54

nirvdrum restored the version-examples branch March 20, 2024 22:02

nirvdrum mentioned this pull request Mar 20, 2024

Version the graph examples #85

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Version the graph examples #84

Version the graph examples #84

nirvdrum commented Mar 15, 2024 •

edited

Loading

rwstauner left a comment

rwstauner Mar 19, 2024

rwstauner Mar 19, 2024

nirvdrum Mar 19, 2024

rwstauner Mar 19, 2024

rwstauner Mar 19, 2024

rwstauner Mar 19, 2024

nirvdrum Mar 19, 2024

rwstauner left a comment

rwstauner Mar 19, 2024

rwstauner left a comment

nirvdrum commented Mar 20, 2024

rwstauner commented Mar 20, 2024

nirvdrum commented Mar 20, 2024

rwstauner commented Mar 20, 2024

	FileUtils.ln_sf("#{__dir__}/../examples/#{graalvm.name}/fib-java.bgv.gz", "#{__dir__}/../examples")
	FileUtils.ln_sf("#{graalvm.name}/fib-java.bgv.gz", "#{__dir__}/../examples")

		FileUtils.ln_sf("#{__dir__}/../examples/#{graalvm.name}/fib-js.bgv.gz", "#{__dir__}/../examples/")
		FileUtils.ln_sf(

Version the graph examples #84

Version the graph examples #84

Conversation

nirvdrum commented Mar 15, 2024 • edited Loading

rwstauner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwstauner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwstauner left a comment

Choose a reason for hiding this comment

nirvdrum commented Mar 20, 2024

rwstauner commented Mar 20, 2024

nirvdrum commented Mar 20, 2024

rwstauner commented Mar 20, 2024

nirvdrum commented Mar 15, 2024 •

edited

Loading