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

Fix GPU's gradient! #74

Merged
merged 2 commits into from
Aug 7, 2023
Merged

Fix GPU's gradient! #74

merged 2 commits into from
Aug 7, 2023

Conversation

paraynaud
Copy link
Member

No description provided.

@paraynaud
Copy link
Member Author

Works on GPU too.

@paraynaud
Copy link
Member Author

paraynaud commented Aug 3, 2023

I will test the last commit on a GPU tomorrow.

edit: It works.

Copy link
Contributor

@farhadrclass farhadrclass left a comment

Choose a reason for hiding this comment

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

I am good with this but should we have a unit test for it (When GPU available the unit test run, Still not sure if GPU is supported on unit test ? )

training_minibatch_iterator = create_minibatch(xtrn, ytrn, size_minibatch)
test_minibatch_iterator = create_minibatch(xtst, ytst, size_minibatch)
current_training_minibatch = rand(training_minibatch_iterator)
current_test_minibatch = rand(test_minibatch_iterator)

nested_array = build_nested_array_from_vec(chain_ANN, x0)
layers_g = similar(params(chain_ANN)) # create a Vector of layer variables
layers_g = similar.(params(chain_ANN)) # create a Vector of layer variables
Copy link
Contributor

Choose a reason for hiding this comment

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

why is param here lower case and in the other src/KnetNLPModels_methods.jl it is Param

Copy link
Contributor

Choose a reason for hiding this comment

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

One thing I am not sure is how to test on GPUin unit test as of now, so I wanted to double check if it is possible

Copy link
Member Author

Choose a reason for hiding this comment

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

why is param here lower case and in the other src/KnetNLPModels_methods.jl it is Param

params is a method and not the type.
The method return a type Param if needed.

One thing I am not sure is how to test on GPUin unit test as of now, so I wanted to double check if it is possible

I pull on the GPU the branch I want to test, and I run manually the tests on the GPU.
Which is nice with KnetNLPModels.jl is that the code works for both CPU and GPU, so i just run ] test once julia in loaded on Atlas.

Copy link
Contributor

Choose a reason for hiding this comment

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

so the test run both GPU and CPU then

@paraynaud
Copy link
Member Author

I am good with this but should we have a unit test for it (When GPU available the unit test run, Still not sure if GPU is supported on unit test ? )

If you want to test on GPU, you must have access to GPU, this is achieved by the job buildkite/knetnlpmodels-dot-jl.
Alexis (mainly) and I tried to make the unit tests work on GPU but Knet fails to install or initialize on the GPU.
Knet.jl has not been updated since the last time I checked, so it fails consistently :/

@farhadrclass
Copy link
Contributor

Can we open a bug here and put it conditional on knet.jl update?
I will approve this branch and we push it with a bug to fix in the future

@farhadrclass
Copy link
Contributor

And I think you mentioned this buildkite/knetnlpmodels-dot-jl fails,
Can we remove it and add it to the bug or fix it so it doesn't fail?

@paraynaud
Copy link
Member Author

Can we open a bug here and put it conditional on knet.jl update? I will approve this branch and we push it with a bug to fix in the future

I don't understand what you mean.
Does bug == github issue ?

I prefer keeping buildkite/knetnlpmodels-dot-jl even if it fails, because Dominique had to ask for us to get access to GPU from buildkite's community.
I'm not sure how it'll react if we remove buildkite/knetnlpmodels-dot-jl.

@farhadrclass
Copy link
Contributor

Sorry for confusion
yes bug is github issue,

Sure if we already expect it to fail and have it in the github issue then I am okay with it

I will pass approve it then, just don''t forget the issue

@paraynaud
Copy link
Member Author

#75

@paraynaud paraynaud merged commit bd672b6 into main Aug 7, 2023
8 of 9 checks passed
@paraynaud paraynaud deleted the pr-fix-grad! branch August 7, 2023 22:46
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