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

Upload Pre-trained Models for Fine Tuning. #896

Merged
merged 1 commit into from
Aug 9, 2016

Conversation

Lucaszw
Copy link
Contributor

@Lucaszw Lucaszw commented Jul 11, 2016

Expects original model definitions from #891

@Lucaszw Lucaszw force-pushed the uploadPretrainedModelForTraining branch 2 times, most recently from a58a8fe to 2f7f75c Compare July 12, 2016 18:12
@Lucaszw Lucaszw removed the caffe label Jul 12, 2016
@IsaacYangSLA
Copy link
Contributor

This will be useful if we later need to store several pre-trained models in/alongside DIGITS.

@Lucaszw
Copy link
Contributor Author

Lucaszw commented Jul 13, 2016

For sure!

</tbody>
<tbody ng-if="jobs.length == 0">
<tr>
<td colspan="{[fields.length]}">
Copy link
Contributor

@jmancewicz jmancewicz Jul 18, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This colspan needs to include the displayed optional columns.

Like

                        {[ colspan = (storage.pretrained_model_fields | filter:show(true)).length + (storage.model_output_fields | filter:show(true)).length;'' ]}
                        <td colspan="{[colspan]}">

If you remove the model_output.fields from the page, remove them from this colspan as well.

screen shot 2016-07-18 at 10 01 36 am

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I will fix this in my next commit.

@jmancewicz
Copy link
Contributor

What is there a mechanism for adding the models to the pre-trained model list?

@Lucaszw
Copy link
Contributor Author

Lucaszw commented Jul 18, 2016

Thanks Joe!

I added an option in the New Model drop-down for uploading a pretrained model. There you select your weights, model def, and labels file. The data corresponds to the pretrained model job after uploading.

@jmancewicz
Copy link
Contributor

Would it make sense to allow it to point to a tgz file, view the table, and have that as an option for uploading models? There might be too much to assume about the role of files, but if it's a model from DIGITS, it should work. Just a thought.

@Lucaszw
Copy link
Contributor Author

Lucaszw commented Jul 19, 2016

That would definitely be a great option to have! I did think think about it, but decided against because of what you mentioned with external models, and also because it expects the original network prototxt file, and this wasn't saved into tar files prior to #891 .

@lukeyeager
Copy link
Member

lukeyeager commented Jul 26, 2016

also because it expects the original network prototxt file, and this wasn't saved into tar files prior to #891

Can't we just check the info.json file for an "original" file and reject the upload if it doesn't exist?

Also, you've got a merge conflict now that we merged #904.

@Lucaszw Lucaszw force-pushed the uploadPretrainedModelForTraining branch from 2f7f75c to 2abae97 Compare July 27, 2016 20:04
@Lucaszw
Copy link
Contributor Author

Lucaszw commented Jul 27, 2016

I added the suggested changes with my latest commit. Everything now should be good to go! :)

@@ -0,0 +1,3 @@
# Copyright (c) 2014-2016, NVIDIA CORPORATION. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should just be "2016" since it's a new file.

@lukeyeager
Copy link
Member

The file upload doesn't look very good. @jmancewicz figured out how to make this work for all browsers: #325

But I can't figure out how to add it to this PR because you're defining the form in javascript!?

@lukeyeager lukeyeager self-assigned this Aug 1, 2016
@lukeyeager
Copy link
Member

  • I don't think it makes sense to display columns like "accuracy" or "loss" for pretrained models on the home page, right?
  • Is there any way to edit the job name after the fact?
  • In a later PR, let's do some validation of the .prototxt before accepting it. It's annoying to get errors like don't set the data_param.source two or three pages after the one where you screwed up. But no need to fix it right away.

@Lucaszw Lucaszw force-pushed the uploadPretrainedModelForTraining branch 2 times, most recently from 7181906 to 7b6fcdb Compare August 1, 2016 22:59
@Lucaszw
Copy link
Contributor Author

Lucaszw commented Aug 1, 2016

Thanks @lukeyeager ! I implemented your comments.

@lukeyeager
Copy link
Member

I tried uploading tarballs from 2 different models and I just get a red "Upload Failed" with no more information.

@Lucaszw Lucaszw force-pushed the uploadPretrainedModelForTraining branch 2 times, most recently from 30412eb to b16f0fe Compare August 2, 2016 17:52
@Lucaszw Lucaszw force-pushed the uploadPretrainedModelForTraining branch from b16f0fe to 1fbf9d7 Compare August 2, 2016 18:01
@Lucaszw
Copy link
Contributor Author

Lucaszw commented Aug 2, 2016

@lukeyeager My bad! The problem should be fixed now. I added some better error messages for upload as well.

@Lucaszw Lucaszw force-pushed the uploadPretrainedModelForTraining branch 2 times, most recently from 45d4a17 to 0b094cb Compare August 4, 2016 23:51
@@ -84,7 +84,7 @@ def __init__(self, **kwargs):
self.solver = None

self.solver_file = CAFFE_SOLVER_FILE
self.original_file = CAFFE_ORIGINAL_FILE
self.model_file = CAFFE_ORIGINAL_FILE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use network_file instead of model_file here? I try to be consistent in calling the .prototxt file a "network description" and not calling it a model unless it has weights attached to it.

The nomenclature I'd like to migrate to (but don't fully support yet) is:

  • "network" for a .prototxt file
  • "model" or "trained model" for a .prototxt and a corresponding .caffemodel file
  • "training" for a group of models

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nevermind. We do the same for Torch. Rats.

However, you will want to be more careful with the upgrade path here.

  • Bump the pickle version
  • In __setstate__, upgrade from original_file to model_file appropriately

@Lucaszw Lucaszw force-pushed the uploadPretrainedModelForTraining branch from 0b094cb to 89bbd5f Compare August 5, 2016 01:38
@Lucaszw
Copy link
Contributor Author

Lucaszw commented Aug 5, 2016

Thanks @lukeyeager ! I made the changes, and tried out the download feature across different model jobs created at different times to test changing from the network name from original_file to model_file .

changed os.rename to shutil.move
@Lucaszw Lucaszw force-pushed the uploadPretrainedModelForTraining branch from 89bbd5f to d365d17 Compare August 5, 2016 19:13
@lukeyeager
Copy link
Member

This still isn't perfect, but it's working well for me and @IsaacYangSLA and we need it to build other functionality on top, so I'm going to merge it as-is.

TODOs:

  • Don't ask for a labels file?
  • Deal with rendering issue with upload button on Ubuntu 16.04 + Firefox 48
  • Make sure uploaded models always show up on the home page without needing refresh (race condition?)

@lukeyeager lukeyeager merged commit 4dd28e6 into NVIDIA:master Aug 9, 2016
@lukeyeager
Copy link
Member

Thanks @Lucaszw!

SlipknotTN pushed a commit to cynnyx/DIGITS that referenced this pull request Mar 30, 2017
…aining

Upload Pre-trained Models for Fine Tuning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants