ci: wart removal #8147
ci: wart removal #8147
Conversation
✅ Deploy Preview for determined-ui ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
rb-determined-ai
force-pushed
the
rb/ci-warts
branch
from
85db2d7
to
9099f01
Compare
| thread.join(timeout=5 * 60) | ||
| if thread.is_alive(): | ||
| # The thread did not exit | ||
| raise ValueError("do_check_logs thread did not exit") |
There was a problem hiding this comment.
If it didn't exit, doesn't that mean that we timed out? Shouldn't we say that instead if that's the case?
There was a problem hiding this comment.
There might be value to printing stdout in this case.
There was a problem hiding this comment.
@MikhailKardash what do you mean? Isn't that what the except Exception clause does, is print the stdout/stderr from the task?
| p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE) | ||
| out, err = p.communicate() | ||
| ret = p.wait() | ||
| if ret: |
| ) | ||
| except socket.timeout: | ||
| raise TimeoutError(f"timed out waiting for {task_type} with id {task_id}") | ||
| result: Any = None |
There was a problem hiding this comment.
nit: since the type of result is Union[bool|Exception], could we make it more explicit and call it
exception: Optional[Exception] = NoneA nit, but one I think makes the flow a tiny bit easier to read.
There was a problem hiding this comment.
I think I was trying to provide positive confirmation that the thread actually set the value... but I don't check for it anywhere, and I only read it after joining the thread... So yeah, good idea, I like it that way.
|
|
||
| thread = threading.Thread(target=do_check_logs, daemon=True) | ||
| thread.start() | ||
| thread.join(timeout=5 * 60) |
There was a problem hiding this comment.
small nit that this stuffs configuration into the implementation.
Options I see, none of which I love (so indeed, maybe it's better as-is):
- set a custom mark and in the test pull it out of
request.node.keywords - abuse
pytest.parameterizeto set a fixture
There was a problem hiding this comment.
honestly I'm inclined to leave it as-is. request.node.keywords is something I've never heard of, which makes me think it would be a lot less readable.
A strategy with 'abuse' in the bullet point sounds undesirable.
My take is that putting config into the implementation is the least evil.
rb-determined-ai
force-pushed
the
rb/ci-warts
branch
from
9099f01
to
df9f0d3
Compare
No, I hadn't, which was sloppy of me. Tests were in fact not coming through, because pytest.fail subclasses BaseException and not Exception. It's fixed now and tested now. Thank you @tayritenour. |
We run some CLI list and describe commands after every single experiment, as a "to sanity check that basic CLI commands don't raise errors". That has made e2e test logs unreadable since the day it was added. For now, we can keep the tests, but don't dump stdout and stderr into the test logs. Some day, we should figure out what codepaths are being tested passively, and write proper tests for them. Also, the test_task_logs has been failing intermittently with a timeout but no error message for weeks. Instead of using pytest.mark.timeout(), which can't be caught, implement our own timeout logic, and only dump stdout and stderr if the cli crashes.
rb-determined-ai
force-pushed
the
rb/ci-warts
branch
from
df9f0d3
to
e8de877
Compare
We run some CLI list and describe commands after every single experiment, as a "to sanity check that basic CLI commands don't raise errors". That has made e2e test logs unreadable since the day it was added. For now, we can keep the tests, but don't dump stdout and stderr into the test logs. Some day, we should figure out what codepaths are being tested passively, and write proper tests for them.
Also, the test_task_logs has been failing intermittently with a timeout but no error message for weeks. Instead of using pytest.mark.timeout(), which can't be caught, implement our own timeout logic, and only dump stdout and stderr if the cli crashes.