Local Microservice for CheckList in Explainability api #436
Conversation
Remove requirements.txt since not required
There was a problem hiding this comment.
Thanks for your PR! I left some comments. Generally, please clean up the code a little bit. I.e. remove commented code that is no longer needed.
Please name the main directory "explainability" (ie drop the "-api").
My understanding is, that this is going to be its own service right? That would mean it needs to be dockerized and integrated into the CI/CD pipeline. Not sure if that is in scope for you. Maybe @Rachneet can comment more.
| adapter = skill["default_skill_args"].get("adapter") | ||
| # extract all tests | ||
| all_tests = [tests["test_cases"] for tests in test_cases] | ||
| # all_tests = list(itertools.chain.from_iterable([tests["test_cases"] for tests in test_cases])) |
| # all_tests = list(itertools.chain.from_iterable([tests["test_cases"] for tests in test_cases])) | ||
| questions, contexts, answers = list(), list(), list() | ||
|
|
||
| test_type = list(itertools.chain.from_iterable([[test["test_type"]] * len(test["test_cases"]) |
There was a problem hiding this comment.
I also like list comprehension in python, but here you loop three times over the same list (test_cases) which can be done in a single loop.
There was a problem hiding this comment.
Thank you! I sucked right there :D
| model_inputs["test_type"] = test_type | ||
| model_inputs["capability"] = capability | ||
| model_inputs["test_name"] = test_name | ||
| # logger.info("inputs:", model_inputs) |
There was a problem hiding this comment.
Use comments only when you feel that some part of the code is hard to understand and commenting will improve readability.
| model_outputs = list() | ||
| try: | ||
| headers = {'Content-type': 'application/json'} | ||
| skill_query_url = f"https://square.ukp-lab.de/api/skill-manager/skill/{skill_id}/query" #note I hardcoded square URL here |
There was a problem hiding this comment.
The url should come from the env variable SQUARE_API_URL
There was a problem hiding this comment.
This part of the code was provided by Haritz. We did not ask him why he hard coded the url but I guess he has some reasons? @HaritzPuerto
There was a problem hiding this comment.
He's on vacation right now. I think it save to use SQUARE_API_URL. The reason we want make this configurable is to be able to deploy to different environments. BTW, the URL contains the protocol, so something like this should work:
import os
# ...
skill_query_url = f"{os.environ['SQUARE_API_URL']}/skill-manager/skill/{skill_id}/query"There was a problem hiding this comment.
Okay great, could you please change the code to that? :D Thanks
| router = APIRouter() | ||
|
|
||
|
|
||
| @router.get('/') |
| @@ -0,0 +1,44 @@ | |||
| [ | |||
There was a problem hiding this comment.
do we need this file checked-in?
There was a problem hiding this comment.
I think so, or else
with open('temp_result/temp_result.json', 'w') as f
will throw an error.
There was a problem hiding this comment.
When using mode="w", open will create a file if it does not exist, or delete it's contents if it does exist. However, it will not create a directory. So you might need to do that first. I would recommend using the builtin [pathlib](https://docs.python.org/3/library/pathlib.html) library. Something like this should work:
from pathlib import Path
output_file = Path(os.environ["OUTPUT_DIR"]) / "result.json"
output_file.parent.mkdir(parents=True, exist_ok=True)
output_file.write_text(json.dumps(result))How does the service further need this resutl?
There was a problem hiding this comment.
I think you can change it however you think it best fit. I'm thinking maybe you can simply remove the parts about a new directory and just call open a new json file. Haritz just told us to save the results locally somewhere so we can use it to analyse it locally. So I just saved it and return it through the api. I'm so sorry if i'm asking too much of you :'(
|
To answer @timbmg, dockerization and integration to CI/CD is not in the scope of this task. But the code should follow the structure of all the other services such that dockerization can be done in a consistent and smooth manner. |
What does this PR do?
Before submitting / marking as 'ready to review'
Who can review?
Feel free to tag members/contributors who may be interested in your PR.