Closed carlosgmartin closed 2 months ago
why are you saying that this PR only partially solves the issues of #1067?
Sorry for the unclear wording. I just meant that #1067 was also addressed by #1068 and #1071.
On my machine, test.sh
creates a file called docs/sg_execution_times.rst
:
Does this happen on your machine? How would you prefer to address it?
Yes, I had seen it as I was doing the review. It's a file created by sphinx gallery. There does not seem to be an option for sphinx gallery to simply not create that file. I would simply add it to the clean up operations, i.e., modify your cleanup function as follows
function cleanup {
deactivate
rm -r "${TEMP_DIR}"
# sphinx gallery creates a log of execution times
# we delete it manually by lack of better mechanism
rm -f docs/sg_execution_times.rst
}
If you have a better solution, I'll take it but I think it's ok to simply clean it up at the end. (Best thing to do would be to open an issue on sphinx-gallery but it's out of scope of this PR).
Thank you again for taking care of this. We are used to run tests on google's side and it will really help users to be able to actually run tests locally properly.
@vroulet We could also add it to .gitignore. Might be useful to keep the file for debugging/inspection purposes. What do you think?
Come to think of it, the same might be true for the virtual env created and used by the test script (keeping it in a gitignored folder, e.g. tests_venv
, rather than deleting it on exit, to allow for debugging/inspection).
We could also add it to .gitignore.
That's the right solution. I don't think we will ever need these logs but I'd prefer to keep test.sh as clean as possible without weird exceptions like this one.
the same might be true for the virtual env
To be honest I don't know why we had this clean up logic in the first place (I'm not one of the original authors of the package). But I don't think either of us are eager to understand why if we start using a gitignore and finding bugs. Your PR is quite clean and I think it solves well the issue you raised so just add docs/sg_execution_times.rst
to the gitignore file and we're good I think.
@vroulet Done.
Partially addresses #1067. Currently,
test.sh
leaves a wheel fileoptax-0.2.4.dev0-py3-none-any.whl
in the root directory of the repo. It also doesn't run cleanup when an intermediate failure occurs.