[go: nahoru, domu]

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

Close temporary files before removal, fix PermissionError #218

Merged
merged 9 commits into from
Dec 1, 2020
Merged

Close temporary files before removal, fix PermissionError #218

merged 9 commits into from
Dec 1, 2020

Conversation

christianversloot
Copy link
Contributor

Fixes #123.

TensorFlow Cloud creates temporary files which it then removes out of neatness reasons, but on Windows a PermissionError emerges because the files haven't been closed before removal.

This PR fixes the issue by closing the temporary files using their file descriptors before attempting to remove them. In doing so, I attempted to keep the existing definitions backwards compatible by making returning the file descriptors optional.

@google-cla
Copy link
google-cla bot commented Oct 16, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Oct 16, 2020
@christianversloot
Copy link
Contributor Author

@googlebot I signed it!

@google-cla google-cla bot added cla: yes and removed cla: no labels Oct 16, 2020
@christianversloot
Copy link
Contributor Author

Hi @pavithrasv - it could be impossible to run TF Cloud on Windows machines at the moment (#123), and this PR provides the fix. Would you have some time to look at it?

@christianversloot
Copy link
Contributor Author

@SinaChavoshi, perhaps?

@pavithrasv
Copy link
Member

This is great, thank you for the PR @christianversloot! I'll take a look at it shortly.

@pavithrasv pavithrasv self-requested a review November 17, 2020 20:23
Copy link
Member
@pavithrasv pavithrasv left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!


Args:
return_descriptors: Whether to return descriptors as well.
"""
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a returns section to this doc string as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

os.remove(preprocessed_entry_point)
for f in container_builder.get_generated_files():
os.remove(f)
for file_path, file_descriptor in container_builder.get_generated_files(return_descriptors=True):
Copy link
Member

Choose a reason for hiding this comment

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

Can you format this so that line length <= 80?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@christianversloot
Copy link
Contributor Author

Thanks for your review! Commits have been attached.

@pavithrasv pavithrasv added the ready to pull PR ready for merge process label Nov 18, 2020
@pavithrasv
Copy link
Member

@christianversloot Can you fix some lint errors?

Line too long (96/80) [line-too-long]
src/python/tensorflow_cloud/core/run.py:197
Trailing whitespace [trailing-whitespace]
src/python/tensorflow_cloud/core/containerize.py:127
Trailing whitespace [trailing-whitespace]
src/python/tensorflow_cloud/core/containerize.py:130
Wrong hanging indentation (add 2 spaces).
(self.docker_file_path, self.docker_file_descriptor),
^ |
[bad-continuation]
src/python/tensorflow_cloud/core/containerize.py:134
Wrong hanging indentation (add 2 spaces).
(self.tar_file_path, self.tar_file_descriptor)
^ |
[bad-continuation]
src/python/tensorflow_cloud/core/containerize.py:135
Trailing whitespace [trailing-whitespace]
src/python/tensorflow_cloud/core/preprocess.py:111
Trailing whitespace [trailing-whitespace]
src/python/tensorflow_cloud/core/preprocess.py:207

@christianversloot
Copy link
Contributor Author
christianversloot commented Nov 20, 2020

@pavithrasv absolutely!

Running pylint locally I think I fixed the errors you mentioned with the attached commit. This one, though, was a bit weird to me:

Trailing whitespace [trailing-whitespace]
src/python/tensorflow_cloud/core/preprocess.py:111

preprocess.py:111 - I could not find a trailing whitespace there.
If it persists, please let me know.

@christianversloot
Copy link
Contributor Author

Hi there, any update? :)

@pavithrasv pavithrasv added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Dec 1, 2020
@pavithrasv
Copy link
Member

Fixed a few more lint errors. I will merge this shortly. Thank you and sorry about the wait!

@copybara-service copybara-service bot merged commit f151383 into tensorflow:master Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes ready to pull PR ready for merge process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Permission Error in Windows
2 participants