Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add with-nlg option for serializing NLG output #140

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

inariksit
Copy link
Contributor

@inariksit inariksit commented Oct 28, 2024

I got the impression that this was needed for @Meowyam to get the NL output to display in the other window.

Two things:

  1. By default: NL output is printed in generated/nlg_en/output.txt
  2. With --nlg-only: only NL output is printed to stdout.

Example run:

$ cabal run lam4-cli -- --nlg-only examples/lottery.l4
A Lottery is Info about a lottery. . Each Lottery has associated with it information like 
   * its total_jackpot ; i.e. how much can be won from the jackpot. 
   * its tax deductible status ; i.e. whether buying tickets from this lottery is tax deductible.

@inariksit inariksit changed the title Output NLG result in generated/nlg_en/output.txt Output NLG result only with --nlg-only Oct 28, 2024
@ym-han ym-han force-pushed the main branch 3 times, most recently from 288a190 to b9c08b3 Compare October 30, 2024 14:24
@ym-han ym-han changed the title Output NLG result only with --nlg-only Add with-nlg option for serializing NLG output Dec 9, 2024
@ym-han
Copy link
Contributor

ym-han commented Dec 9, 2024

I have revised the CLI so that it's a with-nlg as opposed to only-nlg

@ym-han
Copy link
Contributor

ym-han commented Dec 10, 2024

@inariksit when you have time, would you be willing to test this out a bit? If it works, then the only thing that needs to be done before we can merge this would be to refactor the lens stuff in the Render module to use optics instead of lens where possible (as per the convention in the rest of the codebase). There's an example of how this can be done in Main.hs, I think, but I can also do this refactoring if you would prefer that.

@ym-han ym-han self-requested a review December 10, 2024 05:50
Copy link
Contributor

@ym-han ym-han left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted in a comment on the PR, there are two things that should be done:

  1. Some testing of my latest changes to the CLI
  2. Refactor lens stuff in Render to use optics where possible (I think there's an example of this in Main.hs)

When testing, make sure that your .env includes the following lines:

COMPILED_SIMALA_OUTPUT_DIR="generated/simala"
NLG_EN_OUTPUT_DIR="generated/nlg_en"
NLG_EN_OUTPUT_FILENAME="nlg_en_output.json"
GF_PORTABLE_GRAMMAR_FORMAT_FILENAME="Lam4.pgf"

@inariksit
Copy link
Contributor Author

@ym-han With refactoring lens to use optics, do you mean this one-liner function? https://github.com/smucclaw/lam4/blob/nlg-output/lam4-backend/src/Lam4/Render/Render.hs#L90

The most similar line in Main.hs I'm seeing is this https://github.com/smucclaw/lam4/blob/nlg-output/lam4-cli/app/Main.hs#L191 , which uses the function regex from Control.Lens.Regex.ByteString.

This is the commit in October where I switched from PCRE to Lens-regexes, would you prefer me to restore the change? f6aa458

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants