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

Ensure next_index available when loading old stored KeyedVectors models #3117

Merged
merged 2 commits into from
Mar 18, 2022

Conversation

gojomo
Copy link
Collaborator

@gojomo gojomo commented Apr 14, 2021

Fix #3114.

@mpenkov
Copy link
Collaborator

mpenkov commented Apr 28, 2021

Do we want to add some unit tests here, to validate the solution and prevent regressions?

@piskvorky piskvorky changed the title fix #3114: ensure next_index available [WIP] Ensure next_index available when loading old stored KeyedVectors models Apr 28, 2021
@gojomo
Copy link
Collaborator Author

gojomo commented Apr 28, 2021

Do we want to add some unit tests here, to validate the solution and prevent regressions?

Yes, per my comment on #3114: #3114 (comment) - but not sure when I'd be able to get to it, so just dropped this 1st quick stab at a maybe-fix here, as a potential building block for others.

@piskvorky
Copy link
Owner

Looks like the first and the last stab :)

Do we merge as-is, or keep waiting for someone to continue? Or close. @gojomo @mpenkov

@piskvorky piskvorky added this to the Next release milestone Feb 19, 2022
@gojomo
Copy link
Collaborator Author

gojomo commented Feb 26, 2022

I think this is sufficiently straightforward & low-risk it can be added without a unit test. We still have giant gaps in rigorous/complete testing of backward-compatibility goals – a well-labeled, growing library of older models whose loadability into newer versions is automatically checked has been discussed, & (once such processes 1st established) wouldn't be too hard to maintain, & would have caught this (& other) issues earlier. (See related ideas in #2967.) But getting that started (& ideally then using the improved rigor to clean out all the junk test files no longer relevant) seems a bigger task than anyone is currently able to prioritize.

@piskvorky piskvorky assigned mpenkov and unassigned gojomo Feb 26, 2022
@piskvorky piskvorky changed the title [WIP] Ensure next_index available when loading old stored KeyedVectors models [MRG] Ensure next_index available when loading old stored KeyedVectors models Feb 26, 2022
@mpenkov mpenkov changed the title [MRG] Ensure next_index available when loading old stored KeyedVectors models Ensure next_index available when loading old stored KeyedVectors models Mar 18, 2022
@mpenkov mpenkov merged commit 766b9e1 into develop Mar 18, 2022
@mpenkov mpenkov deleted the #3114-next_index branch March 18, 2022 14:10
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.

AttributeError: 'KeyedVectors' object has no attribute 'next_index'
3 participants