-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[BUGFIX] Fixing integration test failures. #3959
[BUGFIX] Fixing integration test failures. #3959
Conversation
uses: actions/setup-python@v2 | ||
with: | ||
python-version: 3.9 | ||
python-version: "3.10" |
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.
Is the python version bump strictly necessary?
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.
@arnavgarg1 It is not; I just felt that testing on the latest -- if all tests pass -- would be better. What do you think?
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.
Fair enough!
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.
This is a lovely change -- thank you!
exception_message: str = 'An Exception inside "pack()" occurred.\n' | ||
exception_traceback: str = traceback.format_exc() | ||
exception_message += f'{type(e).__name__}: "{str(e)}". Traceback: "{exception_traceback}".' | ||
sys.stderr.write(exception_message) |
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.
Is it possible to use the logging directory here? Or do we actually need to write to sys stderr when we're raising it through the ValueError anyway?
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.
@arnavgarg1 I was thinking of showing these messages on the screen, so that in GitHub log review, we can see exactly what went wrong. We could create a directory, but that might hide the failure error messages. It was very challenging to troubleshoot even this one, because the code is wrapping Rust. Happy to discuss, of course. Thanks!
Scope
We have had consistent failures from two tests involving image processing. Through troubleshooting, the cause was determined to be the running out of storage space on the test host Linux machine in GitHub actions. The fixes are:
Code Pull Requests
Please provide the following:
Documentation Pull Requests
Note that the documentation HTML files are in
docs/
while the Markdown sources are inmkdocs/docs
.If you are proposing a modification to the documentation you should change only the Markdown files.
api.md
is automatically generated from the docstrings in the code, so if you want to change something in that file, first modifyludwig/api.py
docstring, then runmkdocs/code_docs_autogen.py
, which will createmkdocs/docs/api.md
.