Adding GPU functionality, enabling data loaders/tuples as input #38

jdunnmon · 2018-09-02T10:05:41Z

Added cuda functionality in Classifier and EndModel
Modifies Classifier and EndModel to give user option to use either DataLoader or tuple as input
Refactors to support the above + small bugfixes (import typos in contrib, etc.)
Adds option for training progress bar for EndModel
Updates multitask class structure to leverage the above
Update tests

…for end model

…Loader input

jdunnmon · 2018-09-02T10:18:08Z

@ajratner @bhancock8 first working cut at adding cuda/dev loader support (have tested it preliminarily on one of my applications). Comments welcome, tried to minimally change the interface (particularly to not change the interface to predict and predict_proba). Chris suggested intelligently linking to embeddings, which I can either add in here or do in the future. Re: tests, I'll see what breaks and update them for dev, o/w lmk if there's any specific ones you'd want to add.

ajratner · 2018-09-06T23:33:08Z

@jdunnmon Can we merge this into master instead? I think we should use dev for major changes to the system, whereas I think we should get smaller functionality additions like this one (even if they change the interface a bit) into master as quickly as possible so people can start using them!

@bhancock8 thoughts on this?

@jdunnmon once you've done this & tests are passing, tag me here so I can review then? Thanks!

jdunnmon · 2018-09-06T23:40:16Z

Yeah, I can prep the PR for master. I'll need to change the tests, etc., but that's on the to do list anyway, just a bit more important to make sure everything's locked down before moving it into master. I'll get that ready.

ajratner · 2018-09-06T23:41:07Z

Cool! I also suggest bc looks like this PR is already pulling in changes from master into dev, so will probably be cleaner anyway!

bhancock8

Are we going to make these same changes for the MultitaskClassifier? Any reason why we wouldn't?

I like the cuda changes. Thanks for adding those.

So the philosophy now is that they either pass in a tuple or a dataloader and we make a dataloader, then internally our train method always gets dataloaders, right?

My main concern with this PR is in the comment on the evaluate method; not sure we need a separate method to essentially just do some pre-formatting and then predict. (Though either way, it makes sense to have a batch[method name] version, of course).

bhancock8 · 2018-09-07T20:05:41Z

metal/classifier.py

+
+        else:
+            raise ValueError(
+                "Unrecognized input data structure, use tuple or DataLoader!"


Not sure we need an exclamation point. :)

Lol, sounds good. Will update

jdunnmon · 2018-09-14T22:22:48Z

@bhancock8 couple thoughts:

(a) can go back and add this in for MultitaskClassifier as well
(b) yes, they pass either a dataloader or a tuple -- if a tuple, we create a dataloader under the hood
(c) The evaluate method was created in order to maintain the API for predict while allowing us to handle batch evaluation, tiebreaking, etc. It seemed messy to have all of that structure for batch vs. non-batch evaluation within score as well as within the cross-validation portion of the training loop. This was also partially done in order to play nicely with higher-level classes so that Classifier doesn't change very much.

@ajratner thoughts on (c)?

bhancock8 · 2018-09-15T19:23:47Z

The more I look at this, the more convinced I am that we don't need a new pair of methods here which duplicate a lot of the functionality of existing methods. I think we just need to add a batch_size kwarg to Classifier.score(), which it will pass down to predict(). And then inside predict() we can either call predict_proba() once or in batches. So any models overwriting predict_proba() don't need to worry about batches, and score just passes it down to predict. What do you think about that?

bhancock8 · 2018-09-15T20:26:29Z

As I've looked at it a little more, I see the issue: if they give us a dataloader with X and Y in score(), then we need to somehow split it before passing to predict(), since it expects only Xs. Here's what I suggest then:

--score() accepts either (X, Y) or a DataLoader. If it's (X, Y), then first thing we make it into a DataLoader. For each batch, it calls predict, stores the results, and then once we have all of Y_p and Y, we calculate metrics once.
--predict() accepts either X or a DataLoader. And again, first thing we convert into a DataLoader, then for each batch, call predict_proba() and do tie-breaking.
--So internally, we're always using DataLoaders (consistent with the train method); we just offer the tuple signature for convenience. And people can do various things with their DataLoaders if they so choose--shuffling, parallel loading, batching, etc.--and we don't care. No need for a batch_size kwarg; if they need batching, they'll submit a DataLoader. Let's have more handled by other people's classes rather than our own. :)

ajratner · 2018-09-16T14:32:55Z

@bhancock8 agreed in full!! @jdunnmon ping me when adjustments made and I'll do a more thorough look then!

jdunnmon · 2018-09-17T05:42:55Z

@bhancock8 @ajratner thanks for the thoughts! think that will work, i'll put in these changes (and update MultitaskClassifier) as soon as I can.

bhancock8 · 2018-09-17T07:13:16Z

Thank you for putting it in! This will be good to have.

…

On Mon 17 Sep 2018 at 07:42, Jared Dunnmon ***@***.***> wrote: @bhancock8 <https://github.com/bhancock8> @ajratner <https://github.com/ajratner> thanks for the thoughts! think that will work, i'll put in these changes (and update MultitaskClassifier) as soon as I can. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#38 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMD2zMZQh5EK47P2CgNSgYEMWOVvlaCbks5ubzZggaJpZM4WWjJC> .

ajratner · 2018-10-12T00:31:07Z

@jdunnmon @bhancock8 Done with my pass here! Take a look when you get a chance?

Only remaining things that would be nice to have:

A test subdirectory for simple GPU functionality testing (not to be run on Travis, but so that a user can check themselves)
Nicer way to handle putting our loss function on CUDA?

ajratner · 2018-10-12T00:31:42Z

Oh also, my tests keep failing due to make check, even though this passes locally (same as JD was seeing). @bhancock8 any ideas here?

… into dev_cuda_loader

ajratner · 2018-10-12T00:57:43Z

@bhancock8 @jdunnmon NVM, I'm pretty sure the formatting errors are coming from master. So should be fine to merge. @bhancock8 when you're up in a few hours, let us know if you can quickly review and approve the changes?

- Tests for LabelModel.score

jdunnmon · 2018-10-12T05:57:40Z

@bhancock8 , ready for your eyes. @ajratner and I went through today and made a bunch of changes to update the above and added a test for successful GPU usage. Alex's _get_predictions function reuses the code I was trying to make sure we could repurpose in evaluate in a more elegant way. Go ahead and give it a look -- want to make any fixes tomorrow morning and get this in!

@ajratner I confirmed that the GPU test runs on my GPU machine and fails on my local mac due to no CUDA existing, so seems like it's doing what it should. Give it a spin tomorrow. If I've used this decorator right, Travis should ignore this test...we'll know soon.

bhancock8

Ok, made a few more comments. I'm going to mark 'approve' so you're not stuck waiting for me on the merge, but do take a look at and address the few suggestions. Nice work!

When you do the merge, move the marker up to v0.2.0, baby!

metal/end_model/loss.py

metal/multitask/mt_classifier.py

bhancock8 · 2018-10-12T09:04:05Z

metal/utils.py

@@ -365,3 +365,13 @@ def slice_data(data, indices):
            return outputs[0]
        else:
            return outputs
+
+
+def mt_to_cuda(data):


I think we need a better name here. I didn't now what "mt" meant. I mean, I assumed multitask, but what does it mean to convert a 'multitask' to cuda?

Can we also add a docstring here saying what the expected type of "data" is?

Also, this current function won't cover X that comes in as a list, right? Will clean up now

tests/gpu/test_gpu.py

metal/classifier.py

bhancock8 · 2018-10-12T09:17:27Z

One more thought: how can the formatting issues be the fault of master @ajratner when it's passing the tests just fine? I think it's something in our additions. Is there a more verbose setting you can use to have it print out what exactly it's upset about or would be changing?

ajratner · 2018-10-12T15:59:58Z

@bhancock8 I think it's because formatting is broken on master

ajratner · 2018-10-12T19:38:14Z

Passes GPU tests as well!

jdunnmon added 8 commits August 28, 2018 16:42

fixed nltk typo, fixed featurizer init in contrib, added gpu support …

2153b73

…for end model

re-add pre commit hook

df50a3d

re-add pre commit hook

1f53c78

updated score method to handle cuda input

7bc4ec0

added disable_prog_bar option in classifier

c3a8e59

Merge branch 'master' into dev_cuda_loader

5dd17b3

changes to end model and classifier to allow for use of tuple or Data…

a68fa17

…Loader input

typo in docstring

d44ff5a

jdunnmon changed the base branch from master to dev September 2, 2018 10:06

jdunnmon added 2 commits September 2, 2018 03:12

removed precommit hook

be8ea6a

re-added pre-commit hook

a4031e4

jdunnmon requested review from ajratner and bhancock8 September 2, 2018 10:15

jdunnmon added 2 commits September 2, 2018 03:31

style fix

362b523

refactor to break ties

7d61e42

bhancock8 reviewed Sep 7, 2018

View reviewed changes

updated multitask classifier

9610ba5

jdunnmon changed the base branch from dev to master October 10, 2018 19:50

jdunnmon added 2 commits October 10, 2018 12:52

doc update

4a608c7

typo

f43d804

ajratner added 3 commits October 11, 2018 17:09

Moved "use_cuda" to global configs

053a473

Adding cuda availability check

2c010dd

Merge branch 'master' into dev_cuda_loader

0f1fa36

ajratner approved these changes Oct 12, 2018

View reviewed changes

ajratner added 3 commits October 11, 2018 17:45

Adding new citation

bf35050

Merge branch 'dev_cuda_loader' of https://github.com/HazyResearch/metal…

3327f2d

… into dev_cuda_loader

Bug fix post-merge

126f55d

jdunnmon and others added 6 commits October 11, 2018 20:33

fixed issues with cuda allocation, training on GPU

e300967

- Handle converting sparse matrices in Classifier.create_dataset

84459db

- Tests for LabelModel.score

Fixed MajorityLabelVoter, added to tests

5d1387e

Cleaning up Basics tutorial

e2beb92

Fixing bug in MTClassifier.score

aa281c6

added gpu test

67fd6f6

bhancock8 approved these changes Oct 12, 2018

View reviewed changes

ajratner added 5 commits October 12, 2018 07:54

Reverting to Y_p

1437541

Cleaned up loss handling of CUDA

20fc090

Docstring fix

f261b33

Cleaning up place_on_gpu function (pending GPU testing)

419d844

Install requirements + README note for GPU tests

607d3bd

ajratner added 2 commits October 12, 2018 09:30

Minor addition of readme to tests/gpu

5646a63

Typo fix

754a63f

ajratner merged commit a2bb278 into master Oct 12, 2018

ajratner deleted the dev_cuda_loader branch October 12, 2018 19:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding GPU functionality, enabling data loaders/tuples as input #38

Adding GPU functionality, enabling data loaders/tuples as input #38

jdunnmon commented Sep 2, 2018 •

edited

Loading

jdunnmon commented Sep 2, 2018 •

edited

Loading

ajratner commented Sep 6, 2018

jdunnmon commented Sep 6, 2018

ajratner commented Sep 6, 2018

bhancock8 left a comment •

edited

Loading

bhancock8 Sep 7, 2018

jdunnmon Sep 14, 2018

jdunnmon commented Sep 14, 2018

bhancock8 commented Sep 15, 2018

bhancock8 commented Sep 15, 2018 •

edited

Loading

ajratner commented Sep 16, 2018

jdunnmon commented Sep 17, 2018

bhancock8 commented Sep 17, 2018 via email

ajratner commented Oct 12, 2018 •

edited by jdunnmon

Loading

ajratner commented Oct 12, 2018

ajratner commented Oct 12, 2018

jdunnmon commented Oct 12, 2018 •

edited

Loading

bhancock8 left a comment

bhancock8 Oct 12, 2018

ajratner Oct 12, 2018

bhancock8 commented Oct 12, 2018

ajratner commented Oct 12, 2018

ajratner commented Oct 12, 2018

Adding GPU functionality, enabling data loaders/tuples as input #38

Adding GPU functionality, enabling data loaders/tuples as input #38

Conversation

jdunnmon commented Sep 2, 2018 • edited Loading

jdunnmon commented Sep 2, 2018 • edited Loading

ajratner commented Sep 6, 2018

jdunnmon commented Sep 6, 2018

ajratner commented Sep 6, 2018

bhancock8 left a comment • edited Loading

Choose a reason for hiding this comment

bhancock8 Sep 7, 2018

Choose a reason for hiding this comment

jdunnmon Sep 14, 2018

Choose a reason for hiding this comment

jdunnmon commented Sep 14, 2018

bhancock8 commented Sep 15, 2018

bhancock8 commented Sep 15, 2018 • edited Loading

ajratner commented Sep 16, 2018

jdunnmon commented Sep 17, 2018

bhancock8 commented Sep 17, 2018 via email

ajratner commented Oct 12, 2018 • edited by jdunnmon Loading

ajratner commented Oct 12, 2018

ajratner commented Oct 12, 2018

jdunnmon commented Oct 12, 2018 • edited Loading

bhancock8 left a comment

Choose a reason for hiding this comment

bhancock8 Oct 12, 2018

Choose a reason for hiding this comment

ajratner Oct 12, 2018

Choose a reason for hiding this comment

bhancock8 commented Oct 12, 2018

ajratner commented Oct 12, 2018

ajratner commented Oct 12, 2018

jdunnmon commented Sep 2, 2018 •

edited

Loading

jdunnmon commented Sep 2, 2018 •

edited

Loading

bhancock8 left a comment •

edited

Loading

bhancock8 commented Sep 15, 2018 •

edited

Loading

ajratner commented Oct 12, 2018 •

edited by jdunnmon

Loading

jdunnmon commented Oct 12, 2018 •

edited

Loading