Skip to content

Merge /api/improve and /api/alternatives, remove custom Herbie properties#1164

Merged
pavpanchekha merged 22 commits intomainfrom
merge-improve-alternatives
Mar 27, 2025
Merged

Merge /api/improve and /api/alternatives, remove custom Herbie properties#1164
pavpanchekha merged 22 commits intomainfrom
merge-improve-alternatives

Conversation

@pavpanchekha
Copy link
Contributor

The core idea behind this PR is that both of these APIs return JSON blobs, and those JSON blobs have distinct sets of keys. So we can just merge both endpoints and return both sets of keys, and everyone will be happy.

Doing this is quite involved, and required:

  • Deleting alternatives in favor of improve
  • Adding alternatives, histories, and derivations keys to make-improve-result
  • Dropping :herbie keys from unparse-result
  • Dropping unparse-result in favor of alternatives (which is an FPCore)
  • Fixing the alternatives key in case of a string name with spaces

@pavpanchekha
Copy link
Contributor Author

This is still WIP, because there are still some non-JSON data structures in the histories portion that need to be JSONified. Ugh.

@pavpanchekha
Copy link
Contributor Author

This continues moving along, there are now only two non-JSON things being returned by alternatives:

  • The test, which unfortunately is widely used
  • The start, which likewise

We really ought to redo this and make it saner to start, but 🤷

@pavpanchekha
Copy link
Contributor Author

The start bit has been fixed, and in fact totally refactored to be saner. The test bit seems to be all that remains

@elmisback
Copy link
Collaborator

I tested the performance of this branch against Odyssey. The default expression works perfectly and it looks like all requests are handled as before but there is a bug with the handling of sqrt(x*x).

After asking for alternatives, the server reports:

racket -y src/main.rkt web --seed 1 --timeout 150 --num-iters 2 \
        --demo --public --prefix /demo/ --port 4053 --save-session www/demo/ \
        --log infra/server.log --quiet 2>&1
write-json: expected argument of type <legal JSON value>; given: 'abs
  context...:
   /usr/share/racket/collects/json/main.rkt:132:2: loop
   [repeats 3 more times]
   /usr/share/racket/collects/json/main.rkt:94:0: write-json*
   /usr/share/racket/pkgs/web-server-lib/web-server/http/response.rkt:179:7
Connection error: flush-output: output port is closed
  output port: #<output-port:tcp-accepted>
  context...:
   /usr/share/racket/pkgs/web-server-lib/web-server/private/dispatch-server-with-connect-unit.rkt:141:2

I guess an 'abs token is ending up in the wrong place? And then the response breaks in the middle for some reason?

Separate issue

Some data is sent back anyway:

{"alternatives":["(FPCore (x) :name \"(sqrt (* x x))\" :precision binary64 :pre TRUE :herbie-spec (sqrt (* x x)) :herbie-preprocess ((abs x)) x)"],"backend":{"end":[{"cost":64,"errors":[... (long list)], ... (some other values, response cuts off on column 430441 in the middle of what look like point-exact pairs)

The response there only has abs(x) in the herbie-preprocess field, so I think we would still need to parse it out to show abs(x) for this case?

(My) issues running Herbie demo

I tried checking this in the Herbie demo for this branch. However, I couldn't get the demo to respond after the initial /improve request, it just calls /check-status forever. I'm probably just running it incorrectly, I just ran racket -y src/main.rkt web and other combinations of flags from the make start-server command.

Odyssey error display/error forwarding from server

When sqrt(x*x) is submitted to /alternatives, the page does nothing and there's an error response in the console:
image

There is in fact no Content-Length header for the response that I can see (though the Content-Length header for the request is 415699). Presumably this means some internal content length requirement was not met because the response was truncated. The actual length of the text sent back is around 430441, and it does break off before it is finished. But it would probably be better if we capture the actual server error.

@pavpanchekha
Copy link
Contributor Author

Ok, there are several issues here:

  • we don't JSONify preprocessing instructions. That's why the server crashed
  • you don't need to parse the preprocessing, you just pass that FPCore back to Herbie to get Tex and Herbie will do that
  • the content length thing is interesting. That would in fact be a constraint I didn't think about

@elmisback
Copy link
Collaborator

  • Great, I'll wait until you've resolved the preprocessing JSON to check Odyssey performance again.
  • That should work fine for the TeX, but Odyssey also converts the FPCore to mathjs using our JS FPCore parser, which doesn't know how to work with the preprocessing info... Maybe we will need to switch the mathjs to also make a server call? Just to make sure I'm understanding correctly, the alternative expression that (partly) came back here for sqrt(x*x) was "x" (with "abs(x)" in the preprocessing field). I think we want to be able to use the FPCore as Odyssey's "true/metadata-complete" representation of the expression (unless we don't?), but here, if we send that FPCore to a different tool, the other tool will operate on "x" rather than "abs(x)" because it doesn't know about herbie-preprocessing. So maybe the question is, do we need to make another call to resolve the result of /alternatives to a "plain old FPCore"?
  • I think the Content-Length error was just an artifact of the server crashing

@pavpanchekha
Copy link
Contributor Author

Yes, I'm coming around to the view that "preprocessing" should be internal to Herbie and not exposed to users.

@pavpanchekha
Copy link
Contributor Author

@elmisback per discussion today, please re-test this branch. I fixed the issue with preprocessing. Making everything live in FPCore is a good goal but for now let's punt that to another PR.

@elmisback
Copy link
Collaborator

Odyssey runs fine when getting alternatives for sqrt(x*x) now, no crashes!

@pavpanchekha pavpanchekha merged commit 1d96ccb into main Mar 27, 2025
6 checks passed
@pavpanchekha pavpanchekha deleted the merge-improve-alternatives branch March 27, 2025 16:15
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.

2 participants