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.

Sign up for GitHub

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

Jump to bottom

Fixes for multiple pretrained model files #851

Merged
merged 3 commits into from
Jun 17, 2016
Merged

Fixes for multiple pretrained model files #851

merged 3 commits into from
Jun 17, 2016

Conversation

lukeyeager
Copy link
Member

@lukeyeager lukeyeager commented Jun 16, 2016 •
edited
Loading

Close #849.

See explanation at #498 (comment).

@lukeyeager lukeyeager added the bug label Jun 16, 2016
@lukeyeager
Copy link
Member Author

@gheinrich can you take a look?

gheinrich
gheinrich reviewed Jun 16, 2016
View reviewed changes
digits/model/tasks/torch_train.py
filenames = self.pretrained_model.split(os.path.pathsep)
if len(filenames) > 1:
raise ValueError('Torch does not support multiple pretrained model files')
args.append('--weights=%s' % self.path(filenames[0]))
Copy link
Contributor

Choose a reason for hiding this comment

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

that looks good though (probably not due to this) it seems the error came too quickly to show in the UI, as if we hadn't yet created the socketIO interface. The error message only showed after I reloaded the page.
I tested that it works when only one pretrained model is provided.

@gheinrich
Copy link
Contributor

This looks good to me. On Linux this is working for me. It's difficult for me to test on Windows as I don't have the set-up. I just verified that os.pathsep returns ; on Windows.

@IsaacYangSLA
Copy link
Contributor

I will test it here on Windows.

@lukeyeager lukeyeager added this to the v4.0.0-rc.2 milestone Jun 16, 2016
@IsaacYangSLA
Copy link
Contributor

Did the new image classification model page remove pre-trained weights field? I couldn't find it on 4.0.0-rc.2.

@lukeyeager
Copy link
Member Author

Did the new image classification model page remove pre-trained weights field?

Look under the "Custom Network" tab

@IsaacYangSLA
Copy link
Contributor

Thanks. My bad. Still a lot to learn about DIGITS.
I tested on Windows. Separating multiple weight files with ';' and path starts with /, both caffe.exe and pycaffe can load those weights. However, there is a bug in _pycaffe_args, so I need to send a PR on loading_script portion of _pycaffe_args.

@lukeyeager
Copy link
Member Author

Great, so we're good? Please merge if so.

@IsaacYangSLA
Copy link
Contributor

Looks great. I believe it works good in Windows now.

@gheinrich gheinrich mentioned this pull request Jun 17, 2016
Merged
@lukeyeager lukeyeager merged commit c90e964 into NVIDIA:master Jun 17, 2016
@lukeyeager lukeyeager deleted the fix-multiple-pretrained-models branch June 17, 2016 16:26
@lukeyeager lukeyeager mentioned this pull request Jun 17, 2016
Closed
SlipknotTN pushed a commit to cynnyx/DIGITS that referenced this pull request Mar 30, 2017
@lukeyeager
Merge pull request NVIDIA#851 from lukeyeager/fix-multiple-pretrained…
3023772 
…-models

Fixes for multiple pretrained model files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug windows
Projects
None yet
Milestone
v4.0.0-rc.2
Development

Successfully merging this pull request may close these issues.
3 participants
@lukeyeager @gheinrich @IsaacYangSLA