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

Optional exception #1906

Merged
merged 23 commits into from Jun 5, 2019

Conversation

Projects
None yet
3 participants
@JohnLangford
Copy link
Member

commented Jun 4, 2019

Tweaked #1897

palmerlao and others added some commits Jun 1, 2019

Add unit tests
- negative test for single-threaded mode
- negative test for regular mode
- positive test for regular mode

The two negative tests are to make sure that any parser threading
still interacts with exception_ptr correctly

@JohnLangford JohnLangford referenced this pull request Jun 4, 2019

Closed

strict parse flag #1897

JohnLangford added some commits Jun 4, 2019

@jackgerrits

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

I do not think rethrowing with an exception pointer is the idiomatic way to do this

@@ -105,6 +105,15 @@ class vw_unrecognised_option_exception : public vw_exception
~vw_unrecognised_option_exception() _NOEXCEPT {}
};

class strict_parse_exception : public std::exception

This comment has been minimized.

Copy link
@jackgerrits

jackgerrits Jun 5, 2019

Member

Should inherit from vw_exception

This comment has been minimized.

Copy link
@JohnLangford

JohnLangford Jun 5, 2019

Author Member

That makes some sense.

@jackgerrits
Copy link
Member

left a comment

I like what this PR is achieving but I would like to object capturing the exception pointer and rethrowing. Just let it bubble up like normal

@JohnLangford

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

Can you use the command line instructions to create your own variation on this patch? (I'm busy with kids right now.)

@jackgerrits

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

Yeah, sure thing

@jackgerrits

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

So I made the changes, but the print statement used for the vw_exception prints the file and line number of the exception. This changes depending on filesystem on every machine and so the test will always fail. Not sure how to resolve that here.

vw (/mnt/c/w/repos/vowpal_wabbit_jackgerrits/vowpalwabbit/parse_example.cc:85): malformed example! '|',space, or EOL expected after : "| x:0.7"in Example #0: "| x:0.7"

See this branch: https://github.com/jackgerrits/vowpal_wabbit/tree/jagerrit/parse_exception

@JohnLangford

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

See comment here: jackgerrits@f42678d#diff-5e0723881d65ba028f55a90faa0f6d93R112

Also note: it looks like you commented out example dealloc -> create memory leak.

Update to vw_exception (#1907)
* Change to vw_exception and bubble up

* fix negate, change file to just name, put back rethrow

* undo change
@jackgerrits

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

I just realized changing the negation script was wrong -- please wait to merge

See #1909

jackgerrits and others added some commits Jun 5, 2019

@JohnLangford JohnLangford merged commit c9be701 into master Jun 5, 2019

7 of 9 checks passed

coverage/coveralls Coverage decreased (-0.2%) to 73.443%
Details
LGTM analysis: C/C++ Running analyses for revisions
Details
LGTM analysis: C# No code changes detected
Details
LGTM analysis: Java No code changes detected
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@JohnLangford

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

Thanks @palmerlao @jackgerrits @rajan-chari

@rajan-chari opening an issue around how we handle errors in parsers seems like a good idea.

@JohnLangford JohnLangford deleted the optional_exception branch Jun 5, 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.