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

custom delft train args #469

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

de-code
Copy link
Collaborator

@de-code de-code commented Jul 26, 2019

Allows custom args to be passed in to the the training operation.

It adds the following optional configuration options:

  • grobid.delft.train.module: Training module (default is grobidTagger.py as per DeLFTModel)
  • grobid.delft.train.args: additional arguments to be passed to the training module. To keep it simple, it's split on spaces (which could be improved, should shell parsing be required in the future).

This is also useful for the current training, to configure the training.

The useELMo flag didn't actually work for training because the command list was immutable.

/cc @kermitt2 @lfoppiano

@coveralls
Copy link

coveralls commented Jul 26, 2019

Coverage Status

Coverage increased (+0.04%) to 38.313% when pulling 95534bc on elifesciences:custom-delft-train-args into 506c00a on kermitt2:master.

@lfoppiano
Copy link
Collaborator

@de-code so this should run the training using delft directly from Grobid?

@de-code
Copy link
Collaborator Author

de-code commented Aug 8, 2019

@de-code so this should run the training using delft directly from Grobid?

Yes, it would be trained like the Wapiti model. Although I have deferred trying to run it that way. Not least because I then need to create GPU version of the Docker container to run in the cloud. Since I am just using a single training dataset at present, it's okay to do that manually (I still need to run the GROBID training command to get the converted training data).

@lfoppiano
Copy link
Collaborator

lfoppiano commented Aug 23, 2019

This PR works in Grobid and on a submodule 👍
Only condition is that the training must be ran after activating the virtual environment.
Better to fix #490 on a separate PR, cause it requires moving around some code.

@lfoppiano lfoppiano self-requested a review August 23, 2019 08:14
@de-code
Copy link
Collaborator Author

de-code commented Nov 29, 2019

Hi @lfoppiano I rebased as requested

@de-code
Copy link
Collaborator Author

de-code commented Nov 29, 2019

BTW I would recommend to always squash merge PRs

@lfoppiano
Copy link
Collaborator

I wonder, what about we make this an optional method for training the delft model?
Since Jep is not reliable at this stage, this could still be removed in future.

For me, personally the alternative is to poke the files from the tmp directory, and run the training manually, which is also not ideal dd

@de-code
Copy link
Collaborator Author

de-code commented Jan 3, 2020

I wonder, what about we make this an optional method for training the delft model?
Since Jep is not reliable at this stage, this could still be removed in future.

For me, personally the alternative is to poke the files from the tmp directory, and run the training manually, which is also not ideal dd

Just trying to go through some old GitHub notifications. Not sure if it was for me but there seem to be some agreement, that a way to train a DeLFT model without GROBID can be beneficial.

For training via GROBID, as you mentioned JEP is less reliable. Maybe training via the command has the additional advantage that it is more transparent, simpler (given the tool already exists) and could be reproduced separately (via the CLI). I guess for training and evaluation there shouldn't be any noticeable performance difference.

@lfoppiano
Copy link
Collaborator

I would parametrize this change via configuration and add a new CLI function in grobid to generate the training data automatically so that we provide multiple ways of training the models.

@lfoppiano
Copy link
Collaborator

lfoppiano commented Jan 23, 2020

@de-code
Copy link
Collaborator Author

de-code commented Nov 25, 2020

I was resolving conflicts after the addition of the architecture flag.
Perhaps this could be merged soonish?

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.

None yet

3 participants