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

Ataymano/c wrapper fix2 #1859

Merged
merged 20 commits into from Jun 4, 2019

Conversation

4 participants
@ataymano
Copy link
Member

commented May 2, 2019

c wrapper:

  • missing feature space construction functions
  • Add label deprecation comments
  • parse_mask usage unification
@@ -69,6 +69,7 @@ class TC_parser
bool* spelling_features;
v_array<char> spelling;
uint32_t hash_seed;
uint64_t parse_mask;

This comment has been minimized.

Copy link
@jackgerrits

jackgerrits May 2, 2019

Member

Would you be able to explain what the parse_mask is for?

@lokitoth lokitoth added this to Required in VW 8.NEXT May 15, 2019

ataymano added some commits May 15, 2019

@@ -120,7 +126,9 @@ extern "C"
VW_DLL_MEMBER float VW_CALLING_CONV VW_Learn(VW_HANDLE handle, VW_EXAMPLE e);
VW_DLL_MEMBER float VW_CALLING_CONV VW_Predict(VW_HANDLE handle, VW_EXAMPLE e);
VW_DLL_MEMBER float VW_CALLING_CONV VW_PredictCostSensitive(VW_HANDLE handle, VW_EXAMPLE e);
//deprecated. Please use either VW_ReadExample for parsing, or VW_ImportExample for example construction

This comment has been minimized.

Copy link
@JohnLangford

JohnLangford Jun 3, 2019

Member

Not deprecated, right?

{ vw * pointer = static_cast<vw*>(handle);
VW::primitive_feature_space * f = reinterpret_cast<VW::primitive_feature_space*>( features );
return static_cast<VW_EXAMPLE>(VW::import_example(*pointer, label, f, len));
}

VW_DLL_MEMBER VW_FEATURE_SPACE VW_CALLING_CONV VW_InitializeFeatureSpaces(size_t len)
{

This comment has been minimized.

Copy link
@JohnLangford

JohnLangford Jun 3, 2019

Member

Why are these needed precisely?

This comment has been minimized.

Copy link
@JohnLangford

JohnLangford Jun 3, 2019

Member

In other words, what is the missing functionality amongst existing interfaces?

This comment has been minimized.

Copy link
@ataymano

ataymano Jun 3, 2019

Author Member

With existing API the only way to "construct" example - VwExample ImportExample(FeatureSpace, Label);
But the only way to get feature space - FeatureSpace ExportExample(VwExample).
So I do not see any way to construct example without introducing functions required for initializing, adding features / namespaces etc to feature space. Please correct me if I am wrong.

This comment has been minimized.

Copy link
@JohnLangford

JohnLangford Jun 3, 2019

Member

I guess the question is: What is the api? Is it vwdll.cpp? Or is it vw.h? To the extent that vwdll.cpp requires additional functions exposing vw.h that seems fine. To the extent that vw.h is missing things, I'd like to understand more deeply. Is vw.h missing things? If not, can these extra interface functions just call the vw.h interface?

This comment has been minimized.

Copy link
@ataymano

ataymano Jun 3, 2019

Author Member

My understanding that cpp api is vw.h while c one is vwdll.
My previous comment was about vwdll.
vw.h is not missing anything - at least allows to create example, since it is exposing full primitive_feature_space implementation:

struct primitive_feature_space // just a helper definition.

So given current vw.h code, direct manipulations with primitive_feature space is technically "using vw.h interface".
vw.h can be refactored to hide its feature_space implementation details behind the interfaces, but still looks like corresponding flat c methods have to be added to vwdll.

This comment has been minimized.

Copy link
@JohnLangford

JohnLangford Jun 3, 2019

Member

Good. So, can we make sure the added vwdll functions use functions in vw.h? Currently, it seems they are not, so this is sort-of creating an alternate interface.

This comment has been minimized.

Copy link
@ataymano

ataymano Jun 3, 2019

Author Member

Sure, let me try.

This comment has been minimized.

Copy link
@ataymano

ataymano Jun 4, 2019

Author Member

Sent the update draft, please take a look.

@JohnLangford

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

This one is breaking on a python test related to lrq which needs to be updated.

I'm also hazy on a detail: why do we need the extra functions in the interface?

It does seem plausible we could easily get this one done quickly.

ataymano@microsoft.com and others added some commits Jun 4, 2019

ataymano@microsoft.com ataymano@microsoft.com
ataymano@microsoft.com ataymano@microsoft.com
@JohnLangford

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

We need to update this test: https://github.com/VowpalWabbit/vowpal_wabbit/blob/master/python/tests/test_sklearn_vw.py#L144 as well, given the implied changes t lrq.

@JohnLangford

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

And it looks like a C# test is failing, although the reason why is unclear... https://ci.appveyor.com/project/JohnLangford/vowpal-wabbit/builds/25040837#L1329

@ataymano

This comment has been minimized.

Copy link
Member Author

commented Jun 4, 2019

And it looks like a C# test is failing, although the reason why is unclear... https://ci.appveyor.com/project/JohnLangford/vowpal-wabbit/builds/25040837#L1329

Yes, I am looking into this - was not able to reproduce locally so far

@lokitoth

This comment has been minimized.

Copy link
Collaborator

commented Jun 4, 2019

And it looks like a C# test is failing, although the reason why is unclear... https://ci.appveyor.com/project/JohnLangford/vowpal-wabbit/builds/25040837#L1329

Yes, I am looking into this - was not able to reproduce locally so far

I have seen some flakiness out of this test in the past.

ataymano@microsoft.com and others added some commits Jun 4, 2019

@JohnLangford JohnLangford merged commit 599f4d3 into VowpalWabbit:master Jun 4, 2019

8 checks passed

LGTM analysis: C# No code changes detected
Details
LGTM analysis: C/C++ No new or fixed alerts
Details
LGTM analysis: Java No code changes detected
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No new or fixed alerts
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.001%) to 73.409%
Details

@jackgerrits jackgerrits moved this from Required to Done in VW 8.NEXT Jun 7, 2019

yarikoptic added a commit to yarikoptic/vowpal_wabbit that referenced this pull request Jun 9, 2019

Merge tag '8.7.0' into releases
* tag '8.7.0': (354 commits)
  Update version to 8.7.0 (VowpalWabbit#1926)
  Fix misconfiguration (VowpalWabbit#1925)
  Ataymano ataymano/warnings fixes (VowpalWabbit#1924)
  Update new version script (VowpalWabbit#1922)
  Run clang-format over codebase (VowpalWabbit#1921)
  change semantics of lambda (VowpalWabbit#1920)
  Bremen79 fix save ftrl (VowpalWabbit#1919)
  fix for daemon race condition (VowpalWabbit#1918)
  Revert "Fix to save the state of FTRL models (VowpalWabbit#1912)" (VowpalWabbit#1916)
  fix static library build (VowpalWabbit#1913)
  more warnings (VowpalWabbit#1915)
  Fix to save the state of FTRL models (VowpalWabbit#1912)
  remove warnings (VowpalWabbit#1911)
  fix closing invalid file descriptor with memory_io_buf (VowpalWabbit#1910)
  Optional exception (VowpalWabbit#1906)
  Contextual Memory Tree (VowpalWabbit#1799)
  Coin betting (VowpalWabbit#1903)
  Ataymano/c wrapper fix2 (VowpalWabbit#1859)
  Use Appveyor MSBuildLogger (VowpalWabbit#1904)
  fix for no label confidence (VowpalWabbit#1901)
  ...

yarikoptic added a commit to yarikoptic/vowpal_wabbit that referenced this pull request Jun 9, 2019

Merge branch 'releases' into dfsg
* releases: (354 commits)
  Update version to 8.7.0 (VowpalWabbit#1926)
  Fix misconfiguration (VowpalWabbit#1925)
  Ataymano ataymano/warnings fixes (VowpalWabbit#1924)
  Update new version script (VowpalWabbit#1922)
  Run clang-format over codebase (VowpalWabbit#1921)
  change semantics of lambda (VowpalWabbit#1920)
  Bremen79 fix save ftrl (VowpalWabbit#1919)
  fix for daemon race condition (VowpalWabbit#1918)
  Revert "Fix to save the state of FTRL models (VowpalWabbit#1912)" (VowpalWabbit#1916)
  fix static library build (VowpalWabbit#1913)
  more warnings (VowpalWabbit#1915)
  Fix to save the state of FTRL models (VowpalWabbit#1912)
  remove warnings (VowpalWabbit#1911)
  fix closing invalid file descriptor with memory_io_buf (VowpalWabbit#1910)
  Optional exception (VowpalWabbit#1906)
  Contextual Memory Tree (VowpalWabbit#1799)
  Coin betting (VowpalWabbit#1903)
  Ataymano/c wrapper fix2 (VowpalWabbit#1859)
  Use Appveyor MSBuildLogger (VowpalWabbit#1904)
  fix for no label confidence (VowpalWabbit#1901)
  ...

yarikoptic added a commit to yarikoptic/vowpal_wabbit that referenced this pull request Jun 9, 2019

Merge branch 'dfsg' into debian
* dfsg: (354 commits)
  Update version to 8.7.0 (VowpalWabbit#1926)
  Fix misconfiguration (VowpalWabbit#1925)
  Ataymano ataymano/warnings fixes (VowpalWabbit#1924)
  Update new version script (VowpalWabbit#1922)
  Run clang-format over codebase (VowpalWabbit#1921)
  change semantics of lambda (VowpalWabbit#1920)
  Bremen79 fix save ftrl (VowpalWabbit#1919)
  fix for daemon race condition (VowpalWabbit#1918)
  Revert "Fix to save the state of FTRL models (VowpalWabbit#1912)" (VowpalWabbit#1916)
  fix static library build (VowpalWabbit#1913)
  more warnings (VowpalWabbit#1915)
  Fix to save the state of FTRL models (VowpalWabbit#1912)
  remove warnings (VowpalWabbit#1911)
  fix closing invalid file descriptor with memory_io_buf (VowpalWabbit#1910)
  Optional exception (VowpalWabbit#1906)
  Contextual Memory Tree (VowpalWabbit#1799)
  Coin betting (VowpalWabbit#1903)
  Ataymano/c wrapper fix2 (VowpalWabbit#1859)
  Use Appveyor MSBuildLogger (VowpalWabbit#1904)
  fix for no label confidence (VowpalWabbit#1901)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.