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

Make the default device for policies "cpu" #828

Closed
wants to merge 1 commit into from

Conversation

ernestum
Copy link
Collaborator

@ernestum ernestum commented Dec 11, 2023

When there is a GPU available, using "auto" as the default device does not seem to be desirable (see #825).

This PR sets the default device to "cpu" which hopefully just works in most cases.

Fixes #825

Copy link

codecov bot commented Dec 11, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (629ef9a) 95.64% compared to head (39647d7) 95.66%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #828      +/-   ##
==========================================
+ Coverage   95.64%   95.66%   +0.01%     
==========================================
  Files         102      102              
  Lines        9655     9655              
==========================================
+ Hits         9235     9236       +1     
+ Misses        420      419       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tomtseng
Copy link
Collaborator

Hmm so the error is this backtrace when running examples/quickstart.py:

  File "examples/quickstart.py", line 83, in <module>
    bc_trainer.train(n_epochs=1)
  File "/home/ttseng/imitation/src/imitation/algorithms/bc.py", line 495, in train
    training_metrics = self.loss_calculator(self.policy, obs_tensor, acts)
  File "/home/ttseng/imitation/src/imitation/algorithms/bc.py", line 130, in __call__
    (_, log_prob, entropy) = policy.evaluate_actions(

On bc.py line 130, tensor_obs is on device auto which assigns it to a GPU, but acts is on the CPU. Could the solution be to move acts onto the same device as tensor_obs? The proposed fix in this PR is to make the device argument default to cpu, but that means that this code still breaks if the user sets the argument to something else

@tomtseng
Copy link
Collaborator

On line 494 of bc.py, right before the failing line 495, we have acts = util.safe_to_tensor(batch["acts"], device=self.policy.device). Despite the device argument seemingly suggesting that acts should be moved to the same device as the policy & observation, but looking at the implementation of util.safe_to_tensor, acts doesn't actually get moved if it's already a tensor. My instinct would be to either change util.safe_to_tensor's behavior, and/or to add a line acts = acts.to(self.policy.device)

@ernestum
Copy link
Collaborator Author

Good point and thanks for the suggestions! I will start a new PR with your proposal.

@ernestum
Copy link
Collaborator Author

Closing this in favor of #831

@ernestum ernestum closed this Dec 15, 2023
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.

Run time Error when run quickstart.py
2 participants