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

Free pointers in parser activations (#4486) #4542

Merged

Conversation

adrianeboyd
Copy link
Contributor

Description

Backport 3dfc764 to v2.1.x:

  • Free pointers in ActivationsC

  • Restructure alloc/free for parser activations

  • Rewrite/restructure to have allocation and free in parallel functions
    in _parser_model rather than partially in _parseC() in Parser.

  • Remove resize_activations from _parser_model.pxd.

Fixes #4538.

Types of change

Checklist

  • I have submitted the spaCy Contributor Agreement.
  • I ran the tests, and all new and existing tests passed.
  • My changes don't require a change to the documentation, or if they do, I've added all required information.

* Free pointers in ActivationsC

* Restructure alloc/free for parser activations

* Rewrite/restructure to have allocation and free in parallel functions
in `_parser_model` rather than partially in `_parseC()` in `Parser`.

* Remove `resize_activations` from `_parser_model.pxd`.
@adrianeboyd adrianeboyd added bug Bugs and behaviour differing from documentation perf / memory Performance: memory use labels Oct 28, 2019
@ines
Copy link
Member

ines commented Oct 28, 2019

Ugh, the test failure here is some pytest version compatibility stuff... I remember this coming up before. Let me see how I fixed this. I think all we need to do is upgrade the pytest requirement.

@adrianeboyd
Copy link
Contributor Author

adrianeboyd commented Oct 28, 2019

Yeah, I ran the tests but probably didn't downgrade pytest.

@honnibal
Copy link
Member

I wouldn't worry about the failure on 3.5 OSX. It's because our wheels don't work there, and it's failing to compile Thinc on that platform. It's fine to release with that test failing.

@ines ines merged commit d677f89 into explosion:v2.1.x Oct 28, 2019
@kabirkhan
Copy link
Contributor

Haha, our time difference is pretty evident here. Thank you all for the quick responses and for getting this out. Really appreciate the fast turnaround!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bugs and behaviour differing from documentation perf / memory Performance: memory use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants