Source location range issue for dictionary

[tugsbayasgalan]

🐛 Bug

During our hackathon today, I ran into this weird error highlighting.

import torch
from typing import Dict

@torch.jit.script
def missing_index(x: Dict[str, int]) -> int:
    return x['dne']

missing_index({'item': 20, 'other_item': 120})

This code outputs:

     9 
     10 
---> 11 missing_index({'item': 20, 'other_item': 120})

RuntimeError: The following operation failed in the TorchScript interpreter.
Traceback of TorchScript (most recent call last):
  File "<ipython-input-6-f7e8ae85d5ad>", line 8, in missing_index
@torch.jit.script
def missing_index(x: Dict[str, int]) -> int:
    return x['dne']
           ~~~~ <--- HERE
RuntimeError: KeyError: dne

I feel like the highlighted range should be either shorter or long enough to highlight the entire statement.

cc @gmagogsfm

[Lilyjjo]

A good place in the codebase to start exploring this would be in pytorch/torch/csrc/jit/frontend/source_range.cpp's void SourceRange::print_with_context function, that controls the logic of what is printed

[Joymfl]

Hey,
I would like to contribute to this issue, but I can't seem to figure out what statement needs to be highlighted in the above example. Any tips would be appreciated.
Thanks.

[Lilyjjo]

Hi @darkuswraith, yes that is a good question! @gmagogsfm @tugsbayasgalan do y'all have opinions on what should be highlighted when a dictionary has a missing key? I'm not sure of the implementation difficulties/differences of the various options below, but I'd be fine with any of these three highlightings:
A: just the dict variable

def missing_index(x: Dict[str, int]) -> int:
    return x['dne']
           ~ <--- HERE

B: just the missing key

def missing_index(x: Dict[str, int]) -> int:
    return x['dne']
             ~~~~~ <--- HERE

C: both the dict variable and the missing key

def missing_index(x: Dict[str, int]) -> int:
    return x['dne']
           ~~~~~~~~ <--- HERE

[gmagogsfm]

@darkuswraith Thanks for contributing to PyTorch!

IMHO, B seems a better choice. @Lilyjjo could you help @darkuswraith with some implementation pointers and code review?

[tugsbayasgalan]

I also think B is better. @Lilyjjo @darkuswraith feel free to tag me on the review as well.

[Joymfl]

Thank you guys for the very prompt and informative replies!
So, from what I understand , I should make it so that the missing key is output in case of an error. So, should I make a separate file that handles this error or should I try and add it to the current code?

[Lilyjjo]

I think adding to the current code is the way to go!

Looking into this task a little bit more, we might be missing the logic to highlight the key access into the dict to create the behavior in option B. @tugsbayasgalan's original code variant prints out like the following:

    return x['dne']
           ~~~~ <--- HERE

But if you change the length of the dict's name the behavior changes:

    return xxxxxx['dneeeeee']
           ~~~~~~ <--- HERE
    return xx['dneeeeee']
           ~~ <--- HERE

Originally I thought the length of the highlighted function was determined by the length of the key name, but it's actually determined by the length of the dict's name. The output looks funky because we have a logic error that messes up when the dict's name is of length 1. I'm not sure if this is an error where the SourceRange is being set, or an error in the function SourceRange::print_with_context() which prints out the SourceRange.

Going the route of option B might is more complicated. For the sake of a first issue, I think fixing the current code to print out Option A for the dicts with name length of 1 correctly is more attainable.

I'd go about this task by determining why the SourceRange start()/end() indexes are only wrong for dicts with the name lengths of 1, and then fixing the logic to work for @tugsbayasgalan's example with option A. Check out SourceRange::print_with_context and the SourceRange definition in torch/csrc/jit/frontend/source_range.h as a starting code point.

Solving option A could be a good warmup task for solving option B :)

[Joymfl]

Thanks! I'll work on this.

[SabbirAhmedAdor629]

🐛 Bug

During our hackathon today, I ran into this weird error highlighting.

import torch
from typing import Dict

@torch.jit.script
def missing_index(x: Dict[str, int]) -> int:
    return x['dne']

missing_index({'item': 20, 'other_item': 120})

This code outputs:

     9 
     10 
---> 11 missing_index({'item': 20, 'other_item': 120})

RuntimeError: The following operation failed in the TorchScript interpreter.
Traceback of TorchScript (most recent call last):
  File "<ipython-input-6-f7e8ae85d5ad>", line 8, in missing_index
@torch.jit.script
def missing_index(x: Dict[str, int]) -> int:
    return x['dne']
           ~~~~ <--- HERE
RuntimeError: KeyError: dne

I feel like the highlighted range should be either shorter or long enough to highlight the entire statement.

cc @gmagogsfm

Hi, I want to work on this. Is it still available