Add better device idx parse checks #37376

ssnl · 2020-04-27T22:19:22Z

Fixes #32079

dr-ci · 2020-04-27T22:34:07Z

💊 CI failures summary and remediations

As of commit 696a87c (more details on the Dr. CI page):

1/1 failures possibly* introduced in this PR
- 1/1 non-CircleCI failure(s)

ci.pytorch.org: 1 failed

Failed: pr/py3.6-clang7-rocmdeb-ubuntu16.04

This comment was automatically generated by Dr. CI (expand for details).

Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker.

See how this bot performed.

This comment has been revised 64 times.

ssnl · 2020-05-03T02:40:43Z

@zou3519 Maybe you could take a look when you have time? :) Thanks!

zou3519 · 2020-05-04T16:08:28Z

c10/core/Device.cpp

@@ -73,12 +73,17 @@ Device::Device(const std::string& device_string) : Device(Type::CPU) {
    type_ = parse_type(s);

    std::string device_index = device_string.substr(index + 1);
+    std::size_t pos = 0;


@ssnl would it be more elegant to do this with std::regex?

pytorch/c10/core/Device.cpp

Lines 41 to 63 in 5ec87a3

// `std::regex` is still in a very incomplete state in GCC 4.8.x,

// so we have to do our own parsing, like peasants.

// https://stackoverflow.com/questions/12530406/is-gcc-4-8-or-earlier-buggy-about-regular-expressions

//

// Replace with the following code once we shed our GCC skin:

//

// static const std::regex regex(

// "(cuda|cpu)|(cuda|cpu):([0-9]+)|([0-9]+)",

// std::regex_constants::basic);

// std::smatch match;

// const bool ok = std::regex_match(device_string, match, regex);

// TORCH_CHECK(ok, "Invalid device string: '", device_string, "'");

// if (match[1].matched) {

// type_ = parse_type_from_string(match[1].str());

// } else {

// if (match[2].matched) {

// type_ = parse_type_from_string(match[1].str());

// } else {

// type_ = Type::CUDA;

// }

// AT_ASSERT(match[3].matched);

// index_ = std::stoi(match[3].str());

// }

. It looks like the minimum version of gcc we support is 5 so this should work now.

To make regex work, we would have to account for future/other device types (ROCM?, etc.). I wasn't sure how to do that properly... So I kept stoi.

You could do something with regex groups, like the following so that we don't hardcode cpu & cuda:

#include <iostream> #include <regex> int main() { static const std::regex with_idx_regex("([a-zA-Z_]+):([0-9]+)"); static const std::regex without_idx_regex("([a-zA-Z_]+)"); std::smatch match; const std::string device_string = "cuda:1"; const bool ok = std::regex_match(device_string, match, with_idx_regex); if (ok) { std::cout << "device: " << match[1].str() << std::endl; std::cout << "idx: " << match[2].str() << std::endl; } }

The regex-based approach will probably have different error messages from the current parsing based approach, but it's a lot nicer than having to maintain our own parser for this

using regex now!

friendly ping :) @zou3519

Thanks! I'll take a look by eod tomorrow

zou3519

lgtm. Could you add an extra test for some two digit device numbers?

c10/core/Device.cpp

facebook-github-bot

@zou3519 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@zou3519 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@zou3519 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot

@zou3519 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

zou3519 · 2020-05-12T17:27:28Z

@ssnl do you mind if I rebase this? There seems to be an internal merge conflict

ssnl · 2020-05-12T17:28:53Z

Done!

…

On Tue, May 12, 2020 at 13:27 Richard Zou ***@***.***> wrote: @ssnl <https://github.com/SsnL> do you mind if I rebase this? There seems to be an internal merge conflict — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#37376 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABLJMZPI3MUZ6XB2ON4A7V3RRGBI7ANCNFSM4MSJ43FA> .

facebook-github-bot

@zou3519 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

zou3519 · 2020-05-13T13:41:35Z

c10/util/string_utils.h

+    // To mimic `std::stoi` and to avoid including `Exception.h`, throw
+    // `std::invalid_argument`.
+    // We can't easily detect out-of-range, so we don't use `std::out_of_range`.
+    throw std::invalid_argument("Not an integer");


I'm getting "error: no member named 'invalid_argument' in namespace 'std'
throw std::invalid_argument("Not an integer");" on some of the internal builds. As far as I can tell we don't need to throw nice errors here, so do you think it would be fine to remove these?

Alternatively we could try to include <stdexcept> but I'm not really sure what the downstream ramifications of that are

Let's try including stdexcept! Without this check stoi(empty_string) would succeed.

facebook-github-bot

@zou3519 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2020-05-14T18:14:02Z

@zou3519 merged this pull request in ae392a7.

ssnl force-pushed the device_parse_error branch from f20a7d2 to 8aa8382 Compare April 27, 2020 22:35

pytorchbot added the open source label Apr 27, 2020

ssnl force-pushed the device_parse_error branch 5 times, most recently from fa6365c to 12acd19 Compare April 28, 2020 05:04

zou3519 self-requested a review May 4, 2020 13:56

zou3519 reviewed May 4, 2020

View reviewed changes

ngimel added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label May 5, 2020

ssnl force-pushed the device_parse_error branch 2 times, most recently from 779d023 to abb6b71 Compare May 5, 2020 18:44

zou3519 approved these changes May 8, 2020

View reviewed changes

c10/core/Device.cpp Show resolved Hide resolved

c10/core/Device.cpp Show resolved Hide resolved

facebook-github-bot reviewed May 8, 2020

View reviewed changes

ssnl force-pushed the device_parse_error branch from 01f09c0 to 4969d75 Compare May 11, 2020 13:42

zou3519 approved these changes May 11, 2020

View reviewed changes

facebook-github-bot reviewed May 11, 2020

View reviewed changes

ssnl force-pushed the device_parse_error branch from 4969d75 to c13b8bd Compare May 11, 2020 13:46

facebook-github-bot reviewed May 11, 2020

View reviewed changes

facebook-github-bot reviewed May 12, 2020

View reviewed changes

ssnl added 7 commits May 12, 2020 13:28

Add better device idx parse checks

60e6bd5

use regex!

277b19d

more tests

273b1fd

add ref

ac3d0b6

fix MSVC check

94a2c90

fix test

02f9fd9

fix test again

64eb2dd

address comments

cb72d55

ssnl force-pushed the device_parse_error branch from c13b8bd to cb72d55 Compare May 12, 2020 17:28

facebook-github-bot reviewed May 12, 2020

View reviewed changes

zou3519 reviewed May 13, 2020

View reviewed changes

include stdexcept

696a87c

facebook-github-bot reviewed May 13, 2020

View reviewed changes

facebook-github-bot closed this in ae392a7 May 14, 2020

facebook-github-bot added the merged label May 14, 2020

ssnl deleted the device_parse_error branch May 14, 2020 19:22

mruberry added the Merged label Oct 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add better device idx parse checks #37376

Add better device idx parse checks #37376

ssnl commented Apr 27, 2020

dr-ci bot commented Apr 27, 2020 •

edited

Loading

ssnl commented May 3, 2020

zou3519 May 4, 2020

ssnl May 5, 2020

zou3519 May 5, 2020

ssnl May 6, 2020

ssnl May 7, 2020

zou3519 May 7, 2020

zou3519 left a comment

facebook-github-bot left a comment

facebook-github-bot left a comment

facebook-github-bot left a comment

facebook-github-bot left a comment

zou3519 commented May 12, 2020

ssnl commented May 12, 2020 via email

facebook-github-bot left a comment

zou3519 May 13, 2020 •

edited

Loading

ssnl May 13, 2020

facebook-github-bot left a comment

facebook-github-bot commented May 14, 2020

	// `std::regex` is still in a very incomplete state in GCC 4.8.x,
	// so we have to do our own parsing, like peasants.
	// https://stackoverflow.com/questions/12530406/is-gcc-4-8-or-earlier-buggy-about-regular-expressions
	//
	// Replace with the following code once we shed our GCC skin:
	//
	// static const std::regex regex(
	// "(cuda\|cpu)\|(cuda\|cpu):([0-9]+)\|([0-9]+)",
	// std::regex_constants::basic);
	// std::smatch match;
	// const bool ok = std::regex_match(device_string, match, regex);
	// TORCH_CHECK(ok, "Invalid device string: '", device_string, "'");
	// if (match[1].matched) {
	// type_ = parse_type_from_string(match[1].str());
	// } else {
	// if (match[2].matched) {
	// type_ = parse_type_from_string(match[1].str());
	// } else {
	// type_ = Type::CUDA;
	// }
	// AT_ASSERT(match[3].matched);
	// index_ = std::stoi(match[3].str());
	// }

Add better device idx parse checks #37376

Add better device idx parse checks #37376

Conversation

ssnl commented Apr 27, 2020

dr-ci bot commented Apr 27, 2020 • edited Loading

💊 CI failures summary and remediations

ci.pytorch.org: 1 failed

ssnl commented May 3, 2020

zou3519 May 4, 2020

Choose a reason for hiding this comment

ssnl May 5, 2020

Choose a reason for hiding this comment

zou3519 May 5, 2020

Choose a reason for hiding this comment

ssnl May 6, 2020

Choose a reason for hiding this comment

ssnl May 7, 2020

Choose a reason for hiding this comment

zou3519 May 7, 2020

Choose a reason for hiding this comment

zou3519 left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

zou3519 commented May 12, 2020

ssnl commented May 12, 2020 via email

facebook-github-bot left a comment

Choose a reason for hiding this comment

zou3519 May 13, 2020 • edited Loading

Choose a reason for hiding this comment

ssnl May 13, 2020

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot commented May 14, 2020

dr-ci bot commented Apr 27, 2020 •

edited

Loading

zou3519 May 13, 2020 •

edited

Loading