Skip to content

fix vision models to use the correct device#274

Merged
Krovatkin merged 4 commits intomasterfrom
krovatkin/fix_cuda
Feb 24, 2021
Merged

fix vision models to use the correct device#274
Krovatkin merged 4 commits intomasterfrom
krovatkin/fix_cuda

Conversation

@Krovatkin
Copy link
Contributor

fix vision models to use the correct device

Copy link
Contributor

@wconstab wconstab left a comment

Choose a reason for hiding this comment

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

Can you also add an assert as @asuhan had described? I'd like to enforce this so it doesn't happen again.

I guess it's not possible to do this outside of the benchmark code as things are structured now. What's the penalty of adding one assert into every measurement? should be a wash, i guess?

@Krovatkin
Copy link
Contributor Author

Krovatkin commented Feb 23, 2021

I guess it's not possible to do this outside of the benchmark code as things are structured now. What's the penalty of adding one assert into every measurement? should be a wash, i guess?

@wconstab
we should follow up in another PR since

  1. for non-vision models, it's a bit tricky to get at models' inputs, let alone models' outputs.
  2. I'm thinking we could do a little bit restructuring to make freezing a little bit easier and hopefully this will let us keep asserts outside of eval at least
    3a if we keep asserts within eval and train, new models might have this exact same issue since folks might not just add them
    3b If we decide to do asserts, I'd still like to do measurements to make sure we aren't affecting a score too much.

@Krovatkin Krovatkin requested a review from wconstab February 23, 2021 22:41
Copy link
Contributor

@wconstab wconstab left a comment

Choose a reason for hiding this comment

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

ok I will not block this then.

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.

4 participants