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

Added compiler params support in keyvi cli #217

Merged
merged 2 commits into from
Feb 21, 2017
Merged

Added compiler params support in keyvi cli #217

merged 2 commits into from
Feb 21, 2017

Conversation

narekgharibyan
Copy link
Contributor

@hendrikmuhs

Fixes #214
Decided do not expose memory_limit param for now and use default to avoid backward compatibility issue later on.

@hendrikmuhs
Copy link
Contributor

@narekgharibyan

A merge conflict is not avoidable as you make use of the API to be changed for 0.2

So if that isn't a super-urgent bug, it would be better to fix it in the 0_2_cleanup branch and get 0.2 ready asap.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.698% when pulling a86e84c on narekgharibyan:keyvicli_compiler_options into f641487 on cliqz-oss:master.

@narekgharibyan
Copy link
Contributor Author

@hendrikmuhs
I didn't get what merge conflicts you meant.

@hendrikmuhs
Copy link
Contributor

I mean this:

dictionary = pykeyvi.KeyOnlyDictionaryCompiler(GB, params)

This is deprecated in 0.2.

Not a big deal, but it means I or you have to change it for 0.2 again. That's why I think non-super-urgent features should go into 0_2_cleanup.

@narekgharibyan
Copy link
Contributor Author

Sure! That's not a problem.
Moreover once we merge this PR into master. I'll merge master into 0.2 branch with appropriate fix so the fix will be already there.

@hendrikmuhs
Copy link
Contributor

ok.

LGTM

@narekgharibyan narekgharibyan merged commit 247efd3 into cliqz-oss:master Feb 21, 2017
@narekgharibyan narekgharibyan deleted the keyvicli_compiler_options branch February 23, 2017 08:53
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