Skip to content
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

Added tempfile for tensorboard logs in tensorboard tests in test_keras_integration.py #761

Merged
merged 3 commits into from
Mar 10, 2022

Conversation

merveenoyan
Copy link
Contributor

@merveenoyan merveenoyan commented Mar 10, 2022

TensorBoard logs are in parent directory of the repository so they weren't removed along with repository, I added lines to remove them so that they don't bug tensorboard tests.

@osanseviero
Copy link
Contributor

Let us know whenever it's ready 😄 also try to re-run the workflow few times to make sure it works.

@merveenoyan
Copy link
Contributor Author

merveenoyan commented Mar 10, 2022

CAME BACK FROM LUNCH 🥑
@osanseviero I created tensorboard log directories under the parent working directory of the repository and not inside the repository itself, and only repo gets removed so the logs remained and hence the error about how the log directory already exists. When I do makedirs (inside working directory/repo_name) before I call push_to_hub_keras the repo is not empty so it doesn't clone there (that's why I didn't do it there as well).
I'm removing the files manually instead.

@merveenoyan merveenoyan changed the title Tensorflow test fix Remove tensorboard logs after tensorboard tests in test_keras_integration.py Mar 10, 2022
@adrinjalali
Copy link
Contributor

I guess here, and in a few other places in the test suite, we actually create files/folders under the working directory. Wouldn't it better to do all of those operations under /tmp and some temporary file/folder names, that never clash between tests?

@merveenoyan
Copy link
Contributor Author

merveenoyan commented Mar 10, 2022

@adrinjalali I actually saw couple of files that exist under tests/fixtures that were left (I don't know if it's on purpose). I removed my temp files directly at the end of the test instead, I can have another temporary directory if you want me to.

@adrinjalali
Copy link
Contributor

adrinjalali commented Mar 10, 2022

Yes I've also seen files under fixture. And they also sometimes end up with very odd permissions. I think the test suite shouldn't create/touch any files under working folder.

Probably a better solution is to create everything (cloning repo, creating tensorboard logs, etc) under a folder you can get from python's tempdir module: https://docs.python.org/3/library/tempfile.html#tempfile.gettempdir

Then you don't even have to remove anything. Everytime you get a new folder, and with every reboot all those files are gone.

@merveenoyan
Copy link
Contributor Author

something along the lines of:

with tempfile.TemporaryDirectory() as tmpdirname:
        
            with open(f"{tmpdirname}/tensorboard.txt", "w") as fp:
                fp.write("Keras FTW")

@merveenoyan merveenoyan requested a review from osanseviero March 10, 2022 13:30
Copy link
Contributor

@osanseviero osanseviero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks for the PR 🚀

We should also rename the PR before merging

@merveenoyan merveenoyan changed the title Remove tensorboard logs after tensorboard tests in test_keras_integration.py Added tempfile for tensorboard logs in tensorboard tests in test_keras_integration.py Mar 10, 2022
@merveenoyan merveenoyan merged commit 7d084d9 into main Mar 10, 2022
@merveenoyan merveenoyan deleted the tensorflow_tests_fix branch March 10, 2022 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants