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

FileObserver setup concurrent-safe #473

Merged
merged 19 commits into from
May 16, 2019
Merged

FileObserver setup concurrent-safe #473

merged 19 commits into from
May 16, 2019

Conversation

dekuenstle
Copy link
Contributor

The FileStorage observer usually works great for me in concurrent environments (e.g. 50 nodes writing simultaneous). Only at the very first runs, some die with an exception because they try to create the storage directories simultaneously. This PR tries to fix this by the following changes:

  • Use os.makedirs with exists_ok=True flag (available for python >= 3.2)
  • Add queued runs the same way than started runs

@coveralls
Copy link

coveralls commented May 4, 2019

Coverage Status

Coverage increased (+0.1%) to 84.04% when pulling 8c4ba3f on dekuenstle:patch-1 into 7263377 on IDSIA:master.

Copy link
Collaborator

@JarnoRFB JarnoRFB left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Just a few small things to change.

# unittest.mock requires python >=3.3
if mock is not None:
with pytest.raises(FileExistsError):
with mock.patch('os.mkdir', mkdir_raises_file_exists):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you might use pytest's monkeypatch here to avoid the extra logic for Python 2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good recommendation, thank you!

.travis.yml Outdated
@@ -14,6 +14,8 @@ matrix:
env: TOX_ENV=tensorflow-2
- python: "3.6"
env: TOX_ENV=flake8
- python: "2.7"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do not think we need extra coverage for py27. I hope to deprecate it soon anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Coveralls did not accept the PR, because some py2 specific paths (e.g. here) were not tested.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it is just that it doesn't matter. We can still merge :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted this commit.

sacred/observers/file_storage.py Outdated Show resolved Hide resolved
if _id is None:
self.dir = tempfile.mkdtemp(prefix='run_', dir=self.basedir)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This whole block could be placed in its own method to increase readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I separated to a new method. Please review whether it still works like you expect.

Copy link
Collaborator

@JarnoRFB JarnoRFB left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! Just one small thing left.

sacred/observers/file_storage.py Outdated Show resolved Hide resolved
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.

None yet

3 participants