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

Allow ACE options in 'process' command #180

Closed
goodmami opened this issue Sep 26, 2018 · 12 comments
Closed

Allow ACE options in 'process' command #180

goodmami opened this issue Sep 26, 2018 · 12 comments
Assignees
Milestone

Comments

@goodmami
Copy link
Member

The delphin process command instantiates an AceParser, AceTransferer, or AceGenerator with the given grammar, but doesn't allow further customization of the process. We could add additional arguments to the process command that are passed along, but this would increase the complexity of the command and may cause collisions in option names. Instead, art's strategy of passing an option string would work, but we'll need to split the string to work with delphin.interfaces.ace's way of handling options. So...

  • create an -o / --options option for the process command (art uses -a, but -a is already taken)
  • use shlex.split() to split the options string into a list
  • pass the list to the cmdargs parameter when instantiating ACE

The -g option is already allowed directly on process, so if it is specified twice (on process and in --options) then maybe we should raise an error. For convenience, we may also want to include -n directly on process (and similarly raise an error if it's specified twice).

@goodmami
Copy link
Member Author

@mcmillanmajora this looks like a good one for you. Note that I've moved the code for commands to delphin.commands, so there's some changes to what's necessary. The -o option handling and shlex.split() parts should stay close to the argument parsing (i.e., in delphin.main), then the process() function in delphin.commands should expect the argument list rather than the string. Maybe the parameter should be called cmdargs to be consistent with AceProcess?

About the -g and -n options: we should not do anything special here for now. Let's just require that -g can only appear on the process command and -n only on the --options string. The code in delphin.interfaces.ace will already check this for us.

@goodmami goodmami added this to the v1.0.0 milestone Nov 14, 2018
@mcmillanmajora
Copy link
Contributor

I think I've got all the pieces connected, but again I'm not quite sure how to test this automatically.

@goodmami
Copy link
Member Author

This one may be harder to test since you're changing the way ACE is called and not handling ACE output. You'd need to do something like mock a test suite and monkeypatch several methods in AceProcess and TestSuite so that the process command returns the list of options. This is pretty involved. For now it's fine if you just verify that it works (by hand) and don't write any tests. We can add them later if needed.

Btw, there is no longer an -a option, so we could use that instead of -o, but I'm pretty ambivalent about these choices.

@arademaker
Copy link
Member

Sorry for the possible silly question:

The delphin process command instantiates an AceParser, AceTransferer, or AceGenerator with the given grammar, but doesn't allow further customization of the process.

What is the Ace Transferer?

@goodmami
Copy link
Member Author

@arademaker these are all classes which manage how PyDelphin calls ACE and interprets its responses. E.g., AceGenerator passes the -e option to ACE so it will generate from MRS inputs, and the outputs are handled differently from parser outputs. AceTransferer is for MRS-to-MRS transfer, e.g., for machine translation or paraphrasing. It requires a transfer grammar, like JaEn.

@goodmami goodmami assigned goodmami and unassigned mcmillanmajora Feb 18, 2019
@goodmami
Copy link
Member Author

Assigning to myself as I have a fix in my branch.

@goodmami goodmami modified the milestones: v1.0.0, v0.9.2 Feb 18, 2019
@arademaker
Copy link
Member

Sorry for another silly question:

art's strategy of passing an option string would work

Can you say more about it ? Are you talking about the way art calls Ace? That is, we need to pass to art the complete ace command line. Right ?

@goodmami
Copy link
Member Author

Yes, but PyDelphin is a bit more restricted. That is, art takes an -a option to give the ACE command that art will use to process items, e.g.:

$ art -a 'ace -g grm.dat -n5 --max-chart-megabytes=2048' my-profile/

And PyDelphin uses -o or --options instead of -a. e.g.:

delphin process -g grm.dat --options='-n5 --max-chart-megabytes=2048' my-profile/

Note the -g option is not part of the string, and some options (e.g., -f) are not allowed, but otherwise it is similar. If calling delphin.commands.process() programmatically instead of through the command-line interface, the options appear in a list, e.g., ['-n5', '--max-charts-megabytes', '2048']

@arademaker
Copy link
Member

arademaker commented Feb 18, 2019

Not sure if I understood the status of the implementation. For me:

$ delphin process -g ~/hpsg/ace/erg.dat --options='-n 500 --timeout=60 --max-words=200 --max-chart-megabytes=4000 --max-unpack-megabytes=5000 --rooted-derivations --udx --disable-generalization' repsol-aa
usage: delphin [-h] [-V] {convert,select,mkprof,process,compare,repp} ...
delphin: error: unrecognized arguments: repsol-aa

leme:tmp ar$ delphin -V
delphin 0.9.1

Options are not allowed yet?

@goodmami
Copy link
Member Author

goodmami commented Feb 18, 2019 via email

@arademaker
Copy link
Member

arademaker commented Feb 19, 2019 via email

@goodmami
Copy link
Member Author

goodmami commented Feb 19, 2019 via email

goodmami added a commit that referenced this issue Apr 1, 2019
resolves #180
resolves #181
resolves #186
fixes #200
fixes #203
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

No branches or pull requests

3 participants