graph-validation-tests integration #10
Conversation
…he can down the road in terms of modelling)
…n into TestHarness
…ng-model update to 0.2.5
…ess - interim commit
…which has high level co-routine calls
…_run.py including a one hop test testrunner (with mock data); probably not yet fully properly coded
…ation with TestHarness 'run_tests' method
requirements-runners.txt
Outdated
| # benchmarks-runner==0.1.3 | ||
| # ui-test-runner==0.0.2 | ||
| # graph-validation-tests==0.0.10 | ||
| -e 'git+https://github.com/TranslatorSRI/graph-validation-tests.git@main#egg=graph-validation-tests' |
There was a problem hiding this comment.
I'd like to stick with packages directly from pypi
There was a problem hiding this comment.
oh, sorry.. that was just a transient situation. I totally agree...
Originally, a fresh graph-validation-tests release was awaiting fresh release of the model (or other package?). I'll review the situation right now and adjust the dependency accordingly.
README.md
Outdated
| - `pip install -r requirements.txt` to install normal dependencies | ||
| - `pip install -r requirements-runners.txt` to install the Test Runners | ||
| - `pip install -r requirements-runners.txt` (or **requirements-runners-win64.txt** using Python 3.12 for _jq_ module compatibility) to install the Test Runners |
There was a problem hiding this comment.
Not necessarily a Windows h8ter, but could we potentially just have a section in the README for instructions on Windows instead of putting binaries and different requirements in the repo?
There was a problem hiding this comment.
I'm not actually a huge Windoze fan myself, just that a Dell workstation running it felt like a good choice the time I needed a new home workstation (I did look at Apple Mac as an alternative but...much higher cost/benefit ratio).
Again, the workarounds here are a bandage .. For sure, we can add a separate README patch recipe section about Windoze.
I spend a couple of hours one day trying to find a Windoze solution to the *nix jq dependency. Found a partial answer which is still problematic.
I think you mentioned that perhaps, the jq wasn't a strict necessity wherever it is used; otherwise, we will need to somehow build acceptable Windoze jq wheels (cached in the project) as a workaround.
There was a problem hiding this comment.
I didn't want to bring it up because it seemed you had put some significant work into getting jq to work, but yes, it is currently not being used (it is used in the UI Test Runner that I made, but we are currently not/can't test the UI at this time).
There was a problem hiding this comment.
HI @maximusunc, I simply attempted to troubleshoot the issue, only partly successfully. If we remove jq from the dependencies (for now), the problem goes away, right?
setup.py
Outdated
| @@ -7,7 +7,7 @@ | |||
|
|
|||
| setup( | |||
| name="sri-test-harness", | |||
| version="0.1.4", | |||
| version="0.1.9", | |||
There was a problem hiding this comment.
Let's bump the minor version
test_harness/slacker.py
Outdated
| @@ -8,9 +8,11 @@ class Slacker: | |||
| def __init__(self, url=None): | |||
| self.url = url if url else os.getenv("SLACK_WEBHOOK_URL") | |||
|
|
|||
| async def post_notification(self, messages=[]): | |||
| async def post_notification(self, messages=None): | |||
There was a problem hiding this comment.
Just for my understanding, what's the advantage of doing it this way? I guess if messages is passed in as None?
There was a problem hiding this comment.
PyCharms complained that the "Default argument value is mutable". I guess replacing it with a Tuple removes this complain?
|
One other knit-picky comment is that we've tried to include the name |
test_harness/run.py
Outdated
|
|
||
| from .reporter import Reporter | ||
| from .slacker import Slacker | ||
|
|
||
|
|
||
| def get_tag(result): |
There was a problem hiding this comment.
I think we're now firmly in the realm of creating a utils file with all these small helper functions.
Sure, I can rename the two graph-validation-tests TestRunner implementations as such. |
| @@ -93,6 +133,20 @@ def cli(): | |||
| help="Path to a file of tests to be run. This would be the same output from downloading the tests via `download_tests()`", | |||
| ) | |||
|
|
|||
| parser.add_argument( | |||
There was a problem hiding this comment.
I think these should have a default value and be required. Either that or include what was passed in the error message inside the graph validation.
|
I got the thing to run (just a few hacks) and the output is an empty dict. |
…pip requirements to latest code.
…d=True plus a default value of None
…unctions to a utils module
|
@maximusunc I've responded to all of your feedback above. Please see the latest set of commits, hence, latest file content. I've not attempted to reconcile the latest code with the master branch. Let me know where things sit now. I'll revisit the test runners tomorrow to see if I can directly get more sensible test output out of them, from some components. |
RichardBruskiewich
dismissed
maximusunc’s stale review
Out of date: previously requested changes have now been made.
There was a problem hiding this comment.
I'm going to make a list of comments:
- If given an unsupported target (i.e. ARS), the validation runner gives a useful message but then doesn't return any results and therefore the Test Harness code errors out.
- Do warnings cause the test to fail, or just errors? Could we get the errors to show up first in the log message dict? Right now they show up after the warnings
- The one hop test runner seems off? I'm sending the same test case to arax and molepro and the former says the subject node is missing and the latter says the object node is missing.
- I think it would be nice to have the trapi and biolink versions have defaults in the CLI
- I mentioned this before, but could we move all the helper functions at the top of run.py into their own utils.py folder and get imported? run.py is just getting a little too big.
|
Hi @maximusunc, I'll iterate on your comments (copied here as a checklist for completion), refactoring accordingly, then post my progress here below, step-by-step, as it is achieved.
|
|
Hi @maximusunc , as mentioned on Slack, I've covered most of your bullet points above with an update of the graph-validation-test-runners package to 0.1.3. I will review your last observation about ARAX and MolePro tomorrow, or soon thereafter. Meanwhile, you can verify whether most of your other concerns are now satisfied. |
|
Closing in favor of #28 |
@maximusunc (@sierra-moxon) here's the latest 'graph-validation-test' mocked up code PR against the TestHarness. I actually have merged against your latest main branch code (release v0.1.9) but haven't take a close look.
It has some relative links across the stack. It mostly uses Pydantic V2 except in reasoner-pydantic (as discussed).
At the moment, despite earnest efforts, I can't seem to single step in PyCharms 'debug' mode to figure out why the test fails in "run" mode...
The graph-validation-tests code seems to work in unit tests in its own repository. There is something else going on here which I do not yet understand...
Tomorrow is another day.