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

[Draft] Refactor example's label and prediction unions #2244

Conversation

jackgerrits
Copy link
Member

@jackgerrits jackgerrits commented Feb 3, 2020

I am exploring a new approach, see #2275 for the first change

  • Make polyprediction and polylabel typesafe, RAII friendly types. With destructors!
    • To access fields it changes from a field to function call
      • In release mode this is essentially no different due to conditionally compiled checks and inlining
    • Convert, audit and test every reduction in VW to conform to the new syntax
      • This is one of the big wins. Types in the label and prediction are now very clear, explicit and intentional. This paves the way for the next big thing in atomization, changing the signature of predict to return a prediction and learn to accept a label as input
      • This process uncovered several bugs where we are interpreting the union as one type when it certainly contains a different type
  • Learners now explicitly set their required label type and prediction type and debug assertions verify examples are well formed coming in and out of reduction. Compiled out of release builds for performance
  • Since polylabel and polyprediction are able to destroy their contained types, I was able to turn almost every type in VW to have scoped destruction, allow moves and copies. Including:
    • v_array
    • example
    • features
    • All labels and prediction types
  • Because of the tagged union, you can delete an example without having to specify what’s in it. Therefore, delete_prediction and delete_label are deprecated and no longer needed
  • Several reductions migrated to RAII types
  • Chance of leaking is now reduced greatly
  • Significantly reduced risk of example reuse bugs

Deprecations

  • vw.h items:
    • copy_example_label
    • free_flatten_example
    • dealloc_example
    • alloc_example
    • copy_example_data
    • copy_example_label
  • v_array items:
    • delete_v
    • v_init
    • pop
    • v_array_contains
  • label_parser items:
    • delete_label
    • copy_label
    • label_size
  • global_data items:
    • delete_prediction

Initial performance

  • ~3% reduction in cb workflow
  • ~1% reduction in RunTests script

Points of interest

  • label.h
  • prediction.h
  • v_array.h

This change exposed this bug #2245

To Do:

  • This change introduces issues with the c api - as it always tries to retrieve the scalar prediction

peterychang and others added 30 commits August 5, 2019 16:02
@lgtm-com
Copy link

lgtm-com bot commented Feb 3, 2020

This pull request introduces 2 alerts when merging 201c721 into 8ccb5ed - view on LGTM.com

new alerts:

  • 1 for Virtual call from constructor or destructor
  • 1 for Declaration hides parameter

@lgtm-com
Copy link

lgtm-com bot commented Feb 3, 2020

This pull request introduces 2 alerts when merging ce91960 into 8ccb5ed - view on LGTM.com

new alerts:

  • 1 for Virtual call from constructor or destructor
  • 1 for Declaration hides parameter

@lgtm-com
Copy link

lgtm-com bot commented Feb 14, 2020

This pull request introduces 3 alerts when merging 513d216 into 7cc5b24 - view on LGTM.com

new alerts:

  • 2 for Mismatching new/free or malloc/delete
  • 1 for Comparison result is always the same

@lgtm-com
Copy link

lgtm-com bot commented Feb 14, 2020

This pull request introduces 3 alerts when merging 22872e6 into 7cc5b24 - view on LGTM.com

new alerts:

  • 2 for Mismatching new/free or malloc/delete
  • 1 for Comparison result is always the same

@jackgerrits
Copy link
Member Author

Note: I am rearchitecting this PR due to perf issues. Do review until it's ready.

@jackgerrits jackgerrits changed the title Refactor example's label and prediction unions [Draft] Refactor example's label and prediction unions Feb 20, 2020
@jackgerrits jackgerrits deleted the jagerrit/untangle_example_union branch April 15, 2021 12:49
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

2 participants