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

Use vendorized portalocker for file locking. #1364

Merged
merged 4 commits into from
Oct 26, 2017
Merged

Use vendorized portalocker for file locking. #1364

merged 4 commits into from
Oct 26, 2017

Conversation

jgosmann
Copy link
Collaborator

@jgosmann jgosmann commented Sep 27, 2017

Motivation and context:
This PR does two things:

  1. It adds some minimal infrastructure to include vendorized dependencies with Nengo.
  2. It replaces our custom file lock implementation with the portalocker library.

The first point is motivated by the second point. The portalocker library uses the respective Windows and Unix API calls to do the file lock which ensures that the lock is released if the process is abnormally killed. Our file lock implementation tried to use the same code on all platforms which makes it hard to correctly handle abnormal terminations (see PR #1309 for a rejected approach).

The vendorizing of third-party libraries, might require us to add the license of those modules in certain places of the documentation, @tbekolay?

I also suspect that other want to chime in how exactly the vendorization workflow should look like. At the moment there is a requirements-vendorized.txt file to enter the vendorized modules into. It lives in nengo/_vendor, but maybe it should be moved to the root folder? In nengo/_vendor is also a readme with instructions. Maybe there is a better place? Let me know what you think.

Interactions with other PRs:
Supersedes #1309.

How has this been tested?
Added tests.

How long should this take to review?

  • Average (neither quick nor lengthy)

Where should a reviewer start?
I'd recommend going commit by commit.

Types of changes:

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have read the CONTRIBUTING.rst document.
  • I have updated the documentation accordingly.
  • I have included a changelog entry.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Still to do:

  • figure out licensing issues
  • Exclude vendorized code from static analysis (and tests?)
  • Fix Windows

@jgosmann jgosmann self-assigned this Sep 28, 2017
@jgosmann
Copy link
Collaborator Author

For some reason this does not work on Windows out of the box ... 😞

@jgosmann
Copy link
Collaborator Author

My suspicion is that the Windows tests fail because of some incompatibility of pytest-runner with multiprocessing (maybe a missing if __name__ == '__main__' guard). The tests produce a weird output that seems to come from setup.py. I tried to directly invoke py.test on AppVeyor, but that leads to ImportMismatchErrors.

Not sure if @tbekolay were quicker to figure this one out.

@jgosmann
Copy link
Collaborator Author

Note, that I can run the tests in a Windows VirtualBox by invoking py.test and they pass.

@jgosmann
Copy link
Collaborator Author

I think I figured it out. Instead of using pytest-runner I had directly invoke py.test and change the install to a develop install to avoid the ImportMismatchErrors. This matches the commands that are used on Travis-CI, so this change makes things more consistent.

Copy link
Member

@tbekolay tbekolay left a comment

Choose a reason for hiding this comment

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

I pushed a commit with some proposed changes, check them out and let me know what you think.

Another thought I had, which I thought you might be able to implement easier than me Jan, is whether the nengo/utils/lock.py file is necessary anymore? It feels like a pretty thin wrapper over what portalocker provides, so if we can remove that file I say go for it.

@jgosmann
Copy link
Collaborator Author

jgosmann commented Oct 2, 2017

  • To deal with the license, I added it to the list of packages that we import, as vendorizing is similar to that (we never modify vendorized packages).

Is there a possibility that we might modify vendorized packages in the future as workaround for bugs that haven't been fixed upstream?

pytest recommends using the pytest script rather than py.test

Do you have a link for that? Just curious whether they provide a reason for that and what that reason is.

Removed the portalocker dist-info directory, and gitignored all dist-info directories. The tests run successfully without the dist-info directory, so I don't believe they're necessary; the main reason to keep them around would be for the license and the version number, but those are both tracked in other locations (LICENSE.rst and requirements.txt, respectively)

If that doesn't confuse pip's install or upgrade process, I'm fine with it.

It would be possible to install a different version of a vendorized package in the _vendor directory than given in requirements.txt (pip install --target nengo/_vendor portalocker==x.y.z). In that case it could be difficult to verify what version is actually installed opposed to the version that is supposed to be installed, but it is not too much of a concern to me.

Another thought I had, which I thought you might be able to implement easier than me Jan, is whether the nengo/utils/lock.py file is necessary anymore? It feels like a pretty thin wrapper over what portalocker provides, so if we can remove that file I say go for it.

I thought about that, but the FileLock and portalocker.Lock APIs are different is some details (e.g. portalocker.Lock can be acquired multiple times, FileLock only once; it can be checked if FileLock is required, but not if portalocker.Lock is acquired). Removing FileLock would mean a number of changes to the cache code with the possibility of introducing new bugs. The FileLock wrapper also allows us to swap out the implementation of the FileLock more easily, even though I'd hope that won't be necessary.

@tbekolay
Copy link
Member

tbekolay commented Oct 2, 2017

Is there a possibility that we might modify vendorized packages in the future as workaround for bugs that haven't been fixed upstream?

Maybe, but we're going to do that I would do the workaround by monkey-patching outside of the _vendor directory, or something like that. I don't suspect it'll come up very often.

Do you have a link for that? Just curious whether they provide a reason for that and what that reason is.

There was some detailed discussion on the pytest-dev mailing list, but I can't find it at the moment... I found this post, and from googling there's this blog post and this SO answer. You can look at the pytest-dev mailing list archive for more.

It would be possible to install a different version of a vendorized package in the _vendor directory than given in requirements.txt (pip install --target nengo/_vendor portalocker==x.y.z).

Sure, but if someone is deep enough in the Nengo source that they're manually installing vendorized dependencies, they probably know they did that. Also, gitignoring those files doesn't remove them, so they will still be there locally for whomever installed the vendorized dependencies, it's just people who pull later that won't have them.

it can be checked if FileLock is required, but not if portalocker.Lock is acquired

OK, thanks for clarifying.

The hope is that this solves some problems with non-released
file locks when the Nengo process gets killed (which can happen
often on HPC clusters).
Seems to be incompatible with multiprocessing and isn't used on
Travis-CI either.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants