-
Notifications
You must be signed in to change notification settings - Fork 17
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 command line interface #48
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some tests for the CLI? There should be some in reviews-labeler that we can use as a guide. It would also be good to merge #43 before merging this, as the travis tests seem to be failing now (probably because I moved it to MantisAI).
Also I don't think we want to add ./results/*.json
to github
nervaluate/nervaluate_cli.py
Outdated
app = typer.Typer() | ||
|
||
|
||
def create_prodigy_spans(doc): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to put these functions into the utils module
nervaluate/nervaluate_cli.py
Outdated
evaluator = Evaluator(true, pred, tags=list(tags.keys())) | ||
global_results, aggregation_results = evaluator.evaluate() | ||
|
||
global_results_path = os.path.join(results_path, "global_results.json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that writing out to files like this is the best way for the CLI. Do we instead want a command which would dump their outputs directly to the console and can then be piped to files, etc? We can then add a flag for the aggregated results, the default being global...or similar? @nsorros @Eleni170
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use wasabi and make the output look similar to spacy and prodigy. Let's provide a file as an argument but by default write to console?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Eleni170 can you include a screenshot of how the results look in the console when you implement so we can also comment on that
Ok so I made some of the changes that @ivyleavedtoadflax suggested. Since there is no logic to create the lists, I removed spaCy for now. I also have to consider passing into the function the lists in a better way than being in a dictionary in a data_path. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good, and we are close. Can we just:
- deal with the dependencies as described in comments
- simplify the CLI so that we only need to type one command instead of two
- Update
nervaluate/__version__.py
by adding authors and incrementing version number. - Reword the commits to match with the schema described in
.gitchangelog.rc
(see https://pypi.org/project/gitchangelog/) so that we can automatically generate a new CHANGELOG.rst
help="If set, will print the results in a pretty format instead of returning the raw json", | ||
), | ||
): | ||
if loader == "conll": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh I get it now! sorry @Eleni170 I hadn't realised why this was necessary. Looks good 👍
typer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think typer
and wasabi
need to go in unpinned_requirements.txt
now that it is a core dependency of nervaluate
. We can then run make update-requirements-txt
to create a pinned requirements.txt
. typer
and wasabi
should also be added to setup.py as a requirement. We also need a Makefile recipe to build requirements.txt
@@ -0,0 +1,63 @@ | |||
import json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would delete nervaluate/cli.py
and rename nervaluate/evaluate.py -> nervaluate/cli.py
. Because this is a very simple cli, we only need one command, which will be nervaluate <arg> <arg>
.
If we create a separate nervaluate/cli.py
then we will need two commands: nervaluate evaluate <arg> <arg>
which probably isn't necessary now that we have decided that this will remain a simple package.
from wasabi import MarkdownRenderer | ||
|
||
from nervaluate import Evaluator | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instantiate app = typer.Typer()
here
|
||
from nervaluate import Evaluator | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set as a command with app.command()
This branch focuses on the creation of a client that can run in command line the evaluator. A standard rvl client is used and it is attempted to be triggered using the command:
rvl nervaluate model_path data_path
The example data that was used are the following:
{"text": "Apple is looking at buying U.K. startup for $1 billion", "meta":[{"label":"ORG", "start":0, "end":5},{"label":"GPE", "start":27, "end":31},{"label":"MONEY", "start":44, "end":54}]}
{"text": "Zuckemberg remaims the Undisputed boss at Facebook.", "meta":[{"label":"PERSON", "start":0, "end":1}, {"label":"ORG", "start":6, "end":7}]}
the results of the evaluation should be added at the 2 json files in the results folder.
Feel free to make comments about the implementation.