-
Notifications
You must be signed in to change notification settings - Fork 0
A new datatractor CLI
#16
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
Conversation
ml-evs
left a comment
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.
Will dig more into this later, just one comment for now!
ml-evs
left a comment
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.
Couple of minor comments!
| try: | ||
| ret = func(**vars(args)) | ||
| if ret is not None and len(ret) > 0: | ||
| print(ret) | ||
| except RuntimeError as e: | ||
| print(e) |
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 for the CLI, we should try to return valid JSON rather than printing Python dictionaries so it can be piped into other tools, e.g.,
| try: | |
| ret = func(**vars(args)) | |
| if ret is not None and len(ret) > 0: | |
| print(ret) | |
| except RuntimeError as e: | |
| print(e) | |
| try: | |
| ret = func(**vars(args)) | |
| if ret is not None and len(ret) > 0: | |
| print(json.dumps(ret)) | |
| except RuntimeError as e: | |
| print(e) |
This might need some better error handling though.
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.
Yeah that's true, I forgot a printed python dict is not 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've added a guarded JSON dump, also when using probe you now get e.g. {"id": "biologic-mpr", "registered_extractors": ["yadg", "galvani"]} back rather than just the list so its valid JSON.
PeterKraus
left a comment
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.
LGTM! (Cannot approve my own PR)
Addresses #10 by implementing
datatractor beam input_type input_path- install & extract from filedatatractor probe input_type- return registered Extractors for thatinput_typedatatractor yard extractor_id- return extractor definition from registrydatatractor install extractor_id- install Extractor (optionally into venv)