comic / evalutils

evalutils helps users create extensions for grand-challenge.org
https://grand-challenge.org
MIT License
23 stars 9 forks source link

[comic#323] general improvements to generated build.sh and test.sh sc… #324

Closed ghost closed 2 years ago

ghost commented 2 years ago

Closes #323

Just some general improvements for the build.sh and test.sh scripts, main items:

  1. use of set -o errexit (same as set -e if you're more familiar with that)
  2. double-quoting around shell variable expansions
  3. use of {} around variable names to clearly delimit them (no bash shell variable manipulation in the {} as of yet)

I ran the tests, not sure what to make of it, they didn't fail, they didn't pass ;)

~/swdev/evalutils$ venv/bin/pytest setup.py test_templates.py
============================================== test session starts ==============================================
platform linux -- Python 3.8.10, pytest-6.2.5, py-1.10.0, pluggy-1.0.0
Using --randomly-seed=2209544794
rootdir: /home/gabyvsdiag/swdev/evalutils, configfile: pytest.ini
plugins: randomly-3.10.1, forked-1.3.0, cov-3.0.0, xdist-2.4.0
gw0 [0] / gw1 [0] / gw2 [0]

============================================= no tests ran in 0.58s =============================================
jmsmkn commented 2 years ago

Unfortunately the files generated from evalutils/templates/evaluation/{{ cookiecutter.package_name }}/test.sh no longer work.

ghost commented 2 years ago

Is that due to quoting around {{cookie-cutter}} ?

Outlook voor Androidhttps://aka.ms/AAb9ysg downloaden


From: James @.> Sent: Wednesday, November 3, 2021 5:32:27 PM To: comic/evalutils @.> Cc: Whitehead, Gaby @.>; Author @.> Subject: Re: [comic/evalutils] [comic#323] general improvements to generated build.sh and test.sh sc… (PR #324)

Unfortunately the files generated from evalutils/templates/evaluation/{{ cookiecutter.package_name }}/test.sh no longer work.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcomic%2Fevalutils%2Fpull%2F324%23issuecomment-959662884&data=04%7C01%7Cgaby.whitehead%40radboudumc.nl%7C40a06daf13c54efa84b308d99ee78733%7Cb208fe69471e48c48d87025e9b9a157f%7C1%7C0%7C637715539513954528%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=TV1SP1P%2B2WVHMlx%2BwM6h4SZ3C%2B4SRxGqqKHty7l6Kjw%3D&reserved=0, or unsubscribehttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAOXNC4IS6OKBBVAAG2GVKCDUKFPZXANCNFSM5HJJ4KEQ&data=04%7C01%7Cgaby.whitehead%40radboudumc.nl%7C40a06daf13c54efa84b308d99ee78733%7Cb208fe69471e48c48d87025e9b9a157f%7C1%7C0%7C637715539513964484%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=232DCN1wwXlwQITSfnlooFK8fVWmaZO22ulw4lYK4pI%3D&reserved=0. Triage notifications on the go with GitHub Mobile for iOShttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fapps.apple.com%2Fapp%2Fapple-store%2Fid1477376905%3Fct%3Dnotification-email%26mt%3D8%26pt%3D524675&data=04%7C01%7Cgaby.whitehead%40radboudumc.nl%7C40a06daf13c54efa84b308d99ee78733%7Cb208fe69471e48c48d87025e9b9a157f%7C1%7C0%7C637715539513974438%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=pl2lGdw1aes4eqPxs%2FAjITtnr3to74sJ4lqwEC0tM0Y%3D&reserved=0 or Androidhttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fplay.google.com%2Fstore%2Fapps%2Fdetails%3Fid%3Dcom.github.android%26referrer%3Dutm_campaign%253Dnotification-email%2526utm_medium%253Demail%2526utm_source%253Dgithub&data=04%7C01%7Cgaby.whitehead%40radboudumc.nl%7C40a06daf13c54efa84b308d99ee78733%7Cb208fe69471e48c48d87025e9b9a157f%7C1%7C0%7C637715539513974438%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=7Y0hTlf1RgjlVF8x6aYXRQ3Et0jxg0juquvysdHenIU%3D&reserved=0.

De informatie in dit bericht is uitsluitend bestemd voor de geadresseerde. Aan dit bericht en de bijlagen kunnen geen rechten worden ontleend. Heeft u deze e-mail onbedoeld ontvangen? Dan verzoeken wij u het te vernietigen en de afzender te informeren. Openbaar maken, kopiëren en verspreiden van deze e-mail of informatie uit deze e-mail is alleen toegestaan met voorafgaande schriftelijke toestemming van de afzender. Het Radboudumc staat geregistreerd bij de Kamer van Koophandel in het handelsregister onder nummer 80262783.

The content of this message is intended solely for the addressee. No rights can be derived from this message or its attachments. If you are not the intended recipient, we kindly request you to delete the message and inform the sender. It is strictly prohibited to disclose, copy or distribute this email or the information inside it, without a written consent from the sender. Radboud university medical center is registered with the Dutch Chamber of Commerce trade register with number 80262783.

jmsmkn commented 2 years ago

Paths by the looks of things:

/home/runner/work/evalutils/evalutils/.tox/py/tmp/popen-gw1/test_evaluation_cli_Segmentati0/testevalSegmentation/test.sh: line 7: ./build.sh: No such file or directory

https://github.com/comic/evalutils/runs/4095165379?check_suite_focus=true

ghost commented 2 years ago

Unfortunately the files generated from evalutils/templates/evaluation/{{ cookiecutter.package_name }}/test.sh no longer work.

Right so this one was a bit strange: the source evaluation...test.sh looked different to the source algortihm....test.sh from the beginning, so I just did the edits manually based on the original. I can recognise the algorithm...test.sh output when I run evalutils, so that is the only one I can test manually (as end user, not via pytest etc). I am not sure what the evaluation... versions are for. I did try running the tests locally but that didn't seem to do much ("0 test runs in ...")

ghost commented 2 years ago
FAILED tests/test_templates.py::test_evaluation_cli[Detection-expected2] - su...
FAILED tests/test_templates.py::test_algorithm_cli[Classification] - subproce...
FAILED tests/test_templates.py::test_algorithm_cli[Segmentation] - subprocess...
FAILED tests/test_templates.py::test_algorithm_cli[Detection] - subprocess.Ca...
FAILED tests/test_templates.py::test_evaluation_cli[Classification-expected0]
FAILED tests/test_templates.py::test_evaluation_cli[Segmentation-expected1]

So the github CI can run the tests, but I can't locally (well, I can run pytest which reports 0 tests have run). I have followed the guidelines in CONTRIBUTING.rst. So what am I missing?

ghost commented 2 years ago

Paths by the looks of things:

/home/runner/work/evalutils/evalutils/.tox/py/tmp/popen-gw1/test_evaluation_cli_Segmentati0/testevalSegmentation/test.sh: line 7: ./build.sh: No such file or directory

https://github.com/comic/evalutils/runs/4095165379?check_suite_focus=true

paths... or the file simply not present? I'll take a look

ghost commented 2 years ago

Yep it does a CD to get then print the parh of the parent (pwd -P) which just seems a rather convoluted way of doing dirname(realname(${0}))

Outlook voor Androidhttps://aka.ms/AAb9ysg downloaden


From: James @.> Sent: Wednesday, November 3, 2021 7:12:54 PM To: comic/evalutils @.> Cc: Whitehead, Gaby @.>; Author @.> Subject: Re: [comic/evalutils] [comic#323] general improvements to generated build.sh and test.sh sc… (PR #324)

@jmsmkn commented on this pull request.


In evalutils/templates/evaluation/{{ cookiecutter.package_name }}/test.shhttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcomic%2Fevalutils%2Fpull%2F324%23discussion_r742211666&data=04%7C01%7Cgaby.whitehead%40radboudumc.nl%7Cf27639cc858e41edc27d08d99ef58f14%7Cb208fe69471e48c48d87025e9b9a157f%7C1%7C0%7C637715599776293818%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=QEmtWZYuG%2B0uP8ozcYwV5l1dUgRmXPl%2B8n1OpxjRcRA%3D&reserved=0:

@@ -1,21 +1,23 @@

!/usr/bin/env bash

-SCRIPTPATH="$( cd "$(dirname "$0")" ; pwd -P )"

This line used to cd, it no longer does. The new L7 probably needs $SCRIPTPATH prepended.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcomic%2Fevalutils%2Fpull%2F324%23pullrequestreview-796932682&data=04%7C01%7Cgaby.whitehead%40radboudumc.nl%7Cf27639cc858e41edc27d08d99ef58f14%7Cb208fe69471e48c48d87025e9b9a157f%7C1%7C0%7C637715599776303773%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=%2B0roUJ2jh26J%2BOIcZYpfR9P6scBeFnY%2Bmie%2BDXQgoGo%3D&reserved=0, or unsubscribehttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAOXNC4JS74MH6BCTTVTEVSDUKF3SNANCNFSM5HJJ4KEQ&data=04%7C01%7Cgaby.whitehead%40radboudumc.nl%7Cf27639cc858e41edc27d08d99ef58f14%7Cb208fe69471e48c48d87025e9b9a157f%7C1%7C0%7C637715599776303773%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=wZT3u%2FPppbwxDEnh7ImuMnQXcPvdgmbq5f8tvjHSLdY%3D&reserved=0. Triage notifications on the go with GitHub Mobile for iOShttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fapps.apple.com%2Fapp%2Fapple-store%2Fid1477376905%3Fct%3Dnotification-email%26mt%3D8%26pt%3D524675&data=04%7C01%7Cgaby.whitehead%40radboudumc.nl%7Cf27639cc858e41edc27d08d99ef58f14%7Cb208fe69471e48c48d87025e9b9a157f%7C1%7C0%7C637715599776313730%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=VKk76qP4bMbqvv9A%2Bt7U4%2BxRbD4ewHSfNcCPGfJ4QR8%3D&reserved=0 or Androidhttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fplay.google.com%2Fstore%2Fapps%2Fdetails%3Fid%3Dcom.github.android%26referrer%3Dutm_campaign%253Dnotification-email%2526utm_medium%253Demail%2526utm_source%253Dgithub&data=04%7C01%7Cgaby.whitehead%40radboudumc.nl%7Cf27639cc858e41edc27d08d99ef58f14%7Cb208fe69471e48c48d87025e9b9a157f%7C1%7C0%7C637715599776323693%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=Qm6HzcqgWQNVw7jPoSO1cFmMOLv2kGm1%2Fi50jtZDO5w%3D&reserved=0.

De informatie in dit bericht is uitsluitend bestemd voor de geadresseerde. Aan dit bericht en de bijlagen kunnen geen rechten worden ontleend. Heeft u deze e-mail onbedoeld ontvangen? Dan verzoeken wij u het te vernietigen en de afzender te informeren. Openbaar maken, kopiëren en verspreiden van deze e-mail of informatie uit deze e-mail is alleen toegestaan met voorafgaande schriftelijke toestemming van de afzender. Het Radboudumc staat geregistreerd bij de Kamer van Koophandel in het handelsregister onder nummer 80262783.

The content of this message is intended solely for the addressee. No rights can be derived from this message or its attachments. If you are not the intended recipient, we kindly request you to delete the message and inform the sender. It is strictly prohibited to disclose, copy or distribute this email or the information inside it, without a written consent from the sender. Radboud university medical center is registered with the Dutch Chamber of Commerce trade register with number 80262783.

jmsmkn commented 2 years ago

Then I don't know, best for you to run evalutils init evaluation and debug the generated script from there.

ghost commented 2 years ago

I think I'm pretty close to getting the pytest stuff running locally which is obviously the preferred choice. If I don't get it running by tonight then I will give up and manually execute all the failing test cases.


From: James @.> Sent: Wednesday, November 3, 2021 7:24:06 PM To: comic/evalutils @.> Cc: Whitehead, Gaby @.>; Author @.> Subject: Re: [comic/evalutils] [comic#323] general improvements to generated build.sh and test.sh sc… (PR #324)

Then I don't know, best for you to run evalutils evaluation init and debug the generated script from there.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcomic%2Fevalutils%2Fpull%2F324%23issuecomment-959804730&data=04%7C01%7Cgaby.whitehead%40radboudumc.nl%7Cc555e51dbc9145f42dcd08d99ef71ff7%7Cb208fe69471e48c48d87025e9b9a157f%7C1%7C0%7C637715606510513724%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=J1g4OLj2APfzeY16l46Nw81pCtVyn3hyAw%2F0YlpScwg%3D&reserved=0, or unsubscribehttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAOXNC4PL3ABD64WVZ7TA7PDUKF44NANCNFSM5HJJ4KEQ&data=04%7C01%7Cgaby.whitehead%40radboudumc.nl%7Cc555e51dbc9145f42dcd08d99ef71ff7%7Cb208fe69471e48c48d87025e9b9a157f%7C1%7C0%7C637715606510523677%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=z%2Fs%2Fj%2BNCB1rqXbzk96BCI6gHGWateBIHZVmfY4KAzpI%3D&reserved=0. Triage notifications on the go with GitHub Mobile for iOShttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fapps.apple.com%2Fapp%2Fapple-store%2Fid1477376905%3Fct%3Dnotification-email%26mt%3D8%26pt%3D524675&data=04%7C01%7Cgaby.whitehead%40radboudumc.nl%7Cc555e51dbc9145f42dcd08d99ef71ff7%7Cb208fe69471e48c48d87025e9b9a157f%7C1%7C0%7C637715606510533634%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=oNPTI9L5JnX2R4kGf8tNXdB1KCUay70TBGDCQNnvnWs%3D&reserved=0 or Androidhttps://eur02.safelinks.protection.outlook.com/?url=https%3A%2F%2Fplay.google.com%2Fstore%2Fapps%2Fdetails%3Fid%3Dcom.github.android%26referrer%3Dutm_campaign%253Dnotification-email%2526utm_medium%253Demail%2526utm_source%253Dgithub&data=04%7C01%7Cgaby.whitehead%40radboudumc.nl%7Cc555e51dbc9145f42dcd08d99ef71ff7%7Cb208fe69471e48c48d87025e9b9a157f%7C1%7C0%7C637715606510533634%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=cFJR6bIRd5JUd5NujZ65Z50gk6VCQAxAgkAZ9mgTszA%3D&reserved=0.

De informatie in dit bericht is uitsluitend bestemd voor de geadresseerde. Aan dit bericht en de bijlagen kunnen geen rechten worden ontleend. Heeft u deze e-mail onbedoeld ontvangen? Dan verzoeken wij u het te vernietigen en de afzender te informeren. Openbaar maken, kopiëren en verspreiden van deze e-mail of informatie uit deze e-mail is alleen toegestaan met voorafgaande schriftelijke toestemming van de afzender. Het Radboudumc staat geregistreerd bij de Kamer van Koophandel in het handelsregister onder nummer 80262783.

The content of this message is intended solely for the addressee. No rights can be derived from this message or its attachments. If you are not the intended recipient, we kindly request you to delete the message and inform the sender. It is strictly prohibited to disclose, copy or distribute this email or the information inside it, without a written consent from the sender. Radboud university medical center is registered with the Dutch Chamber of Commerce trade register with number 80262783.

ghost commented 2 years ago

I have pytest working locally.

Path issue with ./build.sh was indeed a path issue, but not as we know it ;) The execution env on github CI is more similar to the execution env (or lack thereof) that you can expect with an at command or cron job. By changing ./build.sh to ${SCRIPTPATH}/build.sh then the build.sh script was found and ran.

Next issue: tests are giving the correct result output cf expected (false_negatives, f1_score etc all as they should be). However tests still failing due to an index error in the following (test_templates.py):

start = [i for i, ln in enumerate(out) if ln == "["]
end = [i for i, ln in enumerate(out) if ln == "]"]
result = json.loads("\n".join(out[start[0] : (end[-1] + 1)]))

Classic error of simply not checking the container sizes first. Interestingly enough the line before these lines is:

assert "Tests successfully passed..." in out

which means that the test themselves are actually passing!

ghost commented 2 years ago

@jmsmkn take a look at the latest two commits:(https://github.com/comic/evalutils/pull/324/commits/a56bdfb9407932527385d533fb24b98f50e0889b)

  1. I worked out why the index error: the output of test.sh is incorrect and the start and end lists were empty, so I added a check for this in test_templates.py
  2. test.sh - I only worked on the one in evalutils/templates/evaluation - cleaned it up a little more and provided some named variables. The first call to docker run is the one that initially fails - return code of 1. I have looked at the command line in the script and I think there appears to be a command argument missing for the docker to run something, or maybe I'm just mis-reading that? Perhaps you could take a look? If we can solve this one, we may well have all the scripts working correctly for CI (provided I port the code changes to the other files also, where appropriate).
jmsmkn commented 2 years ago

I'm going to close this as there have been no updates for several months, but please feel free to re-open if the issues are resolved.