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

Contextual Memory Tree #1799

Merged
merged 57 commits into from Jun 5, 2019

Conversation

Projects
None yet
3 participants
@wensun
Copy link
Contributor

commented Mar 20, 2019

contextual memory tree (CMT) implementation with scripts located under demo/memory_tree

@JohnLangford

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

This pull request introduces 9 alerts when merging ff39bab into 5a8c495 - view on LGTM.com

new alerts:

  • 6 for Unused import
  • 2 for Multiplication result converted to larger type
  • 1 for Variable defined multiple times

Comment posted by LGTM.com

@JohnLangford

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

This pull request introduces 2 alerts when merging 8b6f5c4 into 5a8c495 - view on LGTM.com

new alerts:

  • 2 for Multiplication result converted to larger type

Comment posted by LGTM.com

@JohnLangford

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

This pull request introduces 2 alerts when merging ac6c146 into 7a5ffbe - view on LGTM.com

new alerts:

  • 2 for Multiplication result converted to larger type

Comment posted by LGTM.com

@JohnLangford

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

The demos are great here, but it seems important to have a test case or two here: https://github.com/VowpalWabbit/vowpal_wabbit/blob/master/test/RunTests which are checked on every operation.

@JohnLangford

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

We should also clean up the lgtm messages before merging to avoid having that build up.

@JohnLangford

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

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

new alerts:

  • 2 for Multiplication result converted to larger type

Comment posted by LGTM.com

Show resolved Hide resolved vowpalwabbit/memory_tree.cc Outdated
Show resolved Hide resolved vowpalwabbit/memory_tree.cc Outdated
Show resolved Hide resolved vowpalwabbit/memory_tree.cc Outdated
Show resolved Hide resolved vowpalwabbit/memory_tree.cc Outdated
Show resolved Hide resolved vowpalwabbit/memory_tree.cc Outdated
Show resolved Hide resolved vowpalwabbit/memory_tree.cc Outdated
Show resolved Hide resolved vowpalwabbit/memory_tree.cc
Show resolved Hide resolved vowpalwabbit/memory_tree.cc Outdated
Show resolved Hide resolved vowpalwabbit/memory_tree.cc Outdated
Show resolved Hide resolved vowpalwabbit/memory_tree.cc Outdated
@JohnLangford

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

I ran out of steam reviewing the code. On the upside, it seems like there is some potential to make it substantially faster :-)

@lokitoth

This comment has been minimized.

Copy link
Collaborator

commented Apr 4, 2019

Hi @wensun,

I wanted to check in on the status of the PR. Will you have a chance to look at John's comments in the next week or two?

@wensun

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

I didn't quite understand this step. Should I create the corresponding stderr files and put the results in them?

Yes. You should run vw with the commandline used in the test, and save the stderr output into the reference file. The way RunTests works is it will do a diff between the two files and if there are significant differences, report an error.

Done.

@JohnLangford

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

It looks like a memory allocation failure: https://travis-ci.org/VowpalWabbit/vowpal_wabbit/builds/541417621#L6839

Decrease the size of the problem/solution? We generally want these tests to run in a sub-second timeframe anyways.

@wensun

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

It looks like a memory allocation failure: https://travis-ci.org/VowpalWabbit/vowpal_wabbit/builds/541417621#L6839

Decrease the size of the problem/solution? We generally want these tests to run in a sub-second timeframe anyways.

I decreased dataset size and the solution bit length, seems no memory allocation failure anymore, but still it didn't pass the travis ci build, and the test failed..any idea why?

@wensun

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

should I delete all std::cout comments?

Wen Sun Wen Sun
@wensun

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

should I delete all std::cout comments?

I think I figured out what's going on here...working on it now.

Wen Sun Wen Sun
@wensun

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

should I delete all std::cout comments?

I think I figured out what's going on here...working on it now.

@wensun

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

now travis CI build is passed, but AppVeyor build still failed. Any idea?

@JohnLangford

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Getting close. I don't know but @lokitoth may.

@JohnLangford

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

Valgrind is unhappy on the code (see below). Can you run with valgrind locally and debug? If you build with "make vw_valgrind", then valgrind will give you precise line numbers for each issue.

I expect the 'uninitialized' warnings here are the cause of the windows build variation. It's common for different compilers to result in different initializations.

jl@jcl-x270u:~/programs/vowpal_wabbit/test$ valgrind ../build/vowpalwabbit/vw train-sets/rcv1_smaller.dat --memory_tree 10 --learn_at_leaf 1 --max_number_of_labels 2 --dream_at_update 0 --dream_repeats 3 --oas 0 --online 1 --leaf_example_multiplier 10 --alpha 0.1 -l 0.001 -b 15 --passes 1 --loss_function squared --holdout_off
==7428== Memcheck, a memory error detector
==7428== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al.
==7428== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info
==7428== Command: ../build/vowpalwabbit/vw train-sets/rcv1_smaller.dat --memory_tree 10 --learn_at_leaf 1 --max_number_of_labels 2 --dream_at_update 0 --dream_repeats 3 --oas 0 --online 1 --leaf_example_multiplier 10 --alpha 0.1 -l 0.001 -b 15 --passes 1 --loss_function squared --holdout_off
==7428==
tree initiazliation is done....
max nodes 10
tree size: 1
max number of unique labels: 2
learn at leaf: 1
num of dream operations per example: 3
current_pass: 0
oas: 0
memory_tree: max_nodes = 10 max_leaf_examples = 33 alpha = 0.1 oas = 0 online =1
Num weight bits = 15
learning rate = 0.001
initial_t = 0
power_t = 0.5
using no cache
Reading datafile = train-sets/rcv1_smaller.dat
num sources = 1
average since example example current current current
loss last counter weight label predict features
1.000000 1.000000 1 1.0 1 0 50
==7428== Invalid read of size 4
==7428== at 0x54DD0C: memory_tree_ns::normalized_linear_prod(memory_tree_ns::memory_tree&, example*, example*) (in /home/jl/programs/vowpal_wabbit/build/vowpalwabbit/vw)
==7428== by 0x54F272: memory_tree_ns::pick_nearest(memory_tree_ns::memory_tree&, LEARNER::learner<char, example>&, unsigned int, example&) (in /home/jl/programs/vowpal_wabbit/build/vowpalwabbit/vw)
==7428== by 0x54FC67: memory_tree_ns::predict(memory_tree_ns::memory_tree&, LEARNER::learner<char, example>&, example&) (in /home/jl/programs/vowpal_wabbit/build/vowpalwabbit/vw)
==7428== by 0x5E092C: vw::learn(example&) (in /home/jl/programs/vowpal_wabbit/build/vowpalwabbit/vw)
==7428== by 0x4B080C: LEARNER::generic_driver(vw&) (in /home/jl/programs/vowpal_wabbit/build/vowpalwabbit/vw)
==7428== by 0x40D804: main (in /home/jl/programs/vowpal_wabbit/build/vowpalwabbit/vw)
==7428== Address 0x787b6b8 is 88 bytes inside a block of size 200 free'd
==7428== at 0x4C2EDEB: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7428== by 0x54DD03: memory_tree_ns::normalized_linear_prod(memory_tree_ns::memory_tree&, example*, example*) (in /home/jl/programs/vowpal_wabbit/build/vowpalwabbit/vw)
==7428== by 0x54F272: memory_tree_ns::pick_nearest(memory_tree_ns::memory_tree&, LEARNER::learner<char, example>&, unsigned int, example&) (in /home/jl/programs/vowpal_wabbit/build/vowpalwabbit/vw)
==7428== by 0x54FC67: memory_tree_ns::predict(memory_tree_ns::memory_tree&, LEARNER::learner<char, example>&, example&) (in /home/jl/programs/vowpal_wabbit/build/vowpalwabbit/vw)
==7428== by 0x5E092C: vw::learn(example&) (in /home/jl/programs/vowpal_wabbit/build/vowpalwabbit/vw)
==7428== by 0x4B080C: LEARNER::generic_driver(vw&) (in /home/jl/programs/vowpal_wabbit/build/vowpalwabbit/vw)
==7428== by 0x40D804: main (in /home/jl/programs/vowpal_wabbit/build/vowpalwabbit/vw)
==7428== Block was alloc'd at
==7428== at 0x4C2FB55: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==7428== by 0x4ECF49: flatten_example(vw&, example*) (in /home/jl/programs/vowpal_wabbit/build/vowpalwabbit/vw)
==7428== by 0x4EE49B: flatten_sort_example(vw&, example*) (in /home/jl/programs/vowpal_wabbit/build/vowpalwabbit/vw)
==7428== by 0x54DAF7: memory_tree_ns::normalized_linear_prod(memory_tree_ns::memory_tree&, example*, example*) (in /home/jl/programs/vowpal_wabbit/build/vowpalwabbit/vw)
==7428== by 0x54F272: memory_tree_ns::pick_nearest(memory_tree_ns::memory_tree&, LEARNER::learner<char, example>&, unsigned int, example&) (in /home/jl/programs/vowpal_wabbit/build/vowpalwabbit/vw)
==7428== by 0x54FC67: memory_tree_ns::predict(memory_tree_ns::memory_tree&, LEARNER::learner<char, example>&, example&) (in /home/jl/programs/vowpal_wabbit/build/vowpalwabbit/vw)
==7428== by 0x5E092C: vw::learn(example&) (in /home/jl/programs/vowpal_wabbit/build/vowpalwabbit/vw)
==7428== by 0x4B080C: LEARNER::generic_driver(vw&) (in /home/jl/programs/vowpal_wabbit/build/vowpalwabbit/vw)
==7428== by 0x40D804: main (in /home/jl/programs/vowpal_wabbit/build/vowpalwabbit/vw)
==7428==

Wen Sun and others added some commits Jun 5, 2019

Wen Sun Wen Sun
.
Wen Sun Wen Sun
Wen Sun Wen Sun
Wen Sun Wen Sun
@wensun

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

Found the issue, now Valgrind doesn't complain anymore,,

@JohnLangford

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

Hmm---the windows test is still failing: https://ci.appveyor.com/project/JohnLangford/vowpal-wabbit/builds/25048636#L1223 Maybe @lokitoth will have an idea tomorrow...

@lokitoth
Copy link
Collaborator

left a comment

It looks like the issue with the Windows tests in this case is that it fails to extract the data file correctly when providing it without "-d"

Show resolved Hide resolved test/RunTests Outdated
Show resolved Hide resolved test/RunTests Outdated

JohnLangford and others added some commits Jun 5, 2019

Update test/RunTests
Co-Authored-By: Jacob Alber <jalber@fernir.com>
Update test/RunTests
Co-Authored-By: Jacob Alber <jalber@fernir.com>

@JohnLangford JohnLangford referenced this pull request Jun 5, 2019

Closed

Cmt bump #1908

@JohnLangford JohnLangford merged commit a4475d5 into VowpalWabbit:master Jun 5, 2019

8 of 9 checks passed

coverage/coveralls Coverage decreased (-0.2%) to 73.326%
Details
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
continuous-integration/travis-ci/push The Travis CI build passed
Details
@JohnLangford

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

Merged at last :-) Thanks @wensun @lokitoth

@wensun

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

Great, thanks @lokitoth, @JohnLangford. I never did a code review before, it's a good experience :)

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.