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

Add RPC for loading a model #1114

Closed
4 tasks done
shonfeder opened this issue Nov 22, 2021 · 8 comments · Fixed by #1855
Closed
4 tasks done

Add RPC for loading a model #1114

shonfeder opened this issue Nov 22, 2021 · 8 comments · Fixed by #1855
Assignees
Labels
effort-medium Can be completed within about 3 days Fserver Apalache server impact-high Advances state of art | ublocks critical work | saves lots oftime

Comments

@shonfeder
Copy link
Contributor

shonfeder commented Nov 22, 2021

Work on this will involve adding the RPC calls for these the following methods, specified in RFC10

  def loadModel(spec: String, aux: List[String] = List()): Either[LoadErr, UninitializedModel]

It will include:

  • Defining the data types
  • Adding the proto specs for the RPC call
  • Calling from the server module into specification loading code
  • Adding unit and integration tests
@shonfeder shonfeder added new New issue to be triaged. Fserver Apalache server labels Nov 22, 2021
@shonfeder shonfeder added this to the Server Mode milestone Dec 13, 2021
@shonfeder shonfeder changed the title [FEATURE] Add RPC for loading a model and initializing constnats [FEATURE] Add RPC for loading a model and initializing constants Apr 19, 2022
@shonfeder shonfeder self-assigned this Jun 7, 2022
@shonfeder shonfeder added effort-medium Can be completed within about 3 days impact-high Advances state of art | ublocks critical work | saves lots oftime and removed new New issue to be triaged. labels Jun 7, 2022
shonfeder pushed a commit that referenced this issue Jun 28, 2022
A light refactor of the `execute` method in the `SanyParserPassImpl`.
I prepared this initially to support #1114, but realized it wasn't quite
the right way to meet the needs of the server.

Still, I think it's a nice enough cleanup to be worth integrating.

The refactor just breaks the execution logic into three chunks with some
helper functions and uses a for comprehension over `Either` to sequence
the possible error cases.
@thpani
Copy link
Collaborator

thpani commented Jul 11, 2022

As discussed in MondApalache, looks like this is blocked by #1761

@shonfeder
Copy link
Contributor Author

I think I should be able to work around for the moment, either by making the tests run in sequence, or manipulating the temp dir system attribute.

@shonfeder
Copy link
Contributor Author

So, after adding a workaround for the race condition of the temp dir, I think I've found the actual problem, which is a deeper issue. Not yet sure if there will be workarounds or if we'll need to submit a fix upstream.

The failure I am seeing from Shai when trying parse two specs concurrently is seems to be related to a detail of the JavaCC parser generator (used by Sany). Here's the error:

ERROR: Second call to constructor of static parser.  You must
       either use ReInit() or set the JavaCC option STATIC to false
       during parser generation.

And the documentation on this in the JavaCC docs:

STATIC: If true, all methods and class variables are specified as static in the generated parser and token manager. This allows only one parser object to be present, but it improves the performance of the parser. To perform multiple parses during one run of your Java program, you will have to call the ReInit() method to reinitialize your parser if it is static. If the parser is non-static, you may use the new operator to construct as many parsers as you wish. These can all be used simultaneously from different threads.

@shonfeder
Copy link
Contributor Author

Looks like the SANY object provides a method to take care of this, so the fix should be simple here :)

@shonfeder
Copy link
Contributor Author

Oh, and in fact that method is getting called at the start of every invocation of the method we are invoking, so I'm back to confusion about the problem here and how to fix it, lol.

So we call

https://github.com/informalsystems/apalache/blob/2a60f813b264ef8e21f80b0c78afa5adad7721b9/tla-io/src/main/scala/at/forsyte/apalache/tla/imp/SanyImporter.scala#L72-L76

and that calls a function meant to set up the global environment on each call:

https://github.com/tlaplus/tlaplus/blob/421bc3d16f869d9c9bb493e5950c445c25c916ea/tlatools/org.lamport.tlatools/src/tla2sany/drivers/SANY.java#L121-L127

So maybe the problem I'm hitting, as @thpani pointed out, that one parse invocation is being called before another has finished.... but I think I'm sequencing the calls.

Will continue investigating.

@shonfeder
Copy link
Contributor Author

shonfeder commented Jul 11, 2022

Ok, so of course we can run the parses in sequence, because we run them in the same VM in our tests (:facepalm:), and confirmed that if I ensure strict sequencing of the parses everything works out as expected:

      testM("cal load two valid specs in sequence") {
        val specA =
          """|---- MODULE A ----
             |Foo == TRUE
             |====
             |""".stripMargin

        val specB =
          """|---- MODULE B ----
             |Foo == TRUE
             |====
             |""".stripMargin

        for {
          s <- service
          conn <- s.openConnection(ConnectRequest())
          respA <- s.loadModel(LoadModelRequest(conn.id, specA))
          respB <- s.loadModel(LoadModelRequest(conn.id, specB))
        } yield assert(respA.result.isSpec && respB.result.isSpec)(isTrue)
      },

So I just need to ensure that we're only allowing one instance of the parser to run at a time.

@shonfeder shonfeder changed the title [FEATURE] Add RPC for loading a model and initializing constants [FEATURE] Add RPC for loading a model Jul 11, 2022
@shonfeder shonfeder changed the title [FEATURE] Add RPC for loading a model [Add RPC for loading a model Jul 11, 2022
@shonfeder shonfeder changed the title [Add RPC for loading a model Add RPC for loading a model Jul 11, 2022
@konnov
Copy link
Collaborator

konnov commented Jul 12, 2022

This looks like a limitation of the SANY parser, which was generated by JavaCC. I am not sure how hard it would be for the tlaplus team to make sure that SANY works in a multithreaded environment. Since parsing works relatively fast in comparison to other passes, I can only think of a very simple solution: introduce a mutex that gets locked on every call to SANY. We would lose the ability to call the parser in parallel, but avoid other unexpected issues.

@thpani
Copy link
Collaborator

thpani commented Jul 12, 2022

Shon's already using a semaphore in #1855. I also agree that this is the best workaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort-medium Can be completed within about 3 days Fserver Apalache server impact-high Advances state of art | ublocks critical work | saves lots oftime
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants