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

Docstring parsing throws errors at runtime for misformatted docstrings #3306

Closed
k-visvanathan opened this issue Nov 30, 2020 · 3 comments
Closed
Assignees
Labels
type: bug Something isn't working

Comments

@k-visvanathan
Copy link

Summary

Dagster uses docstring-parser to inspect docstrings and obtain additional information about solid inputs and outputs. This results in a runtime error being thrown in instances where docstring format may deviate slightly from google-style guidelines. Docstring checking should at the very least be configurable and not throw runtime errors.

Reproduction

Valid docstring:

Args:
    arg_name: description of the arg

Invalid docstring (contains hyphen instead of colon):

Args:    
    arg_name - description of arg 

The second example results in the following error when trying to run the solid the docstring is enclosed in:

Traceback (most recent call last):
  File "dagster/core/definitions/decorators/solid.py", line 59, in __call__
    else infer_input_definitions_for_solid(self.name, fn)
  File "dagster/core/definitions/inference.py", line 132, in infer_input_definitions_for_solid
    descriptions = _infer_input_description_from_docstring(fn)
  File "dagster/core/definitions/inference.py", line 20, in _infer_input_description_from_docstring
    docstring = parse(fn.__doc__)
  File "docstring_parser/parser.py", line 20, in parse
    rets.append(parse_(text))
  File "docstring_parser/google.py", line 272, in parse
    return GoogleParser().parse(text)
  File "docstring_parser/google.py", line 262, in parse
    ret.meta.append(self._build_meta(part, title))
  File "docstring_parser/google.py", line 106, in _build_meta
    before, desc = text.split(":", 1)
ValueError: not enough values to unpack (expected 2, got 1)
@k-visvanathan k-visvanathan added the type: bug Something isn't working label Nov 30, 2020
@catherinewu
Copy link
Contributor

catherinewu commented Dec 1, 2020

Hey @k-visvanathan thanks for the feedback! Agreed that this should not cause runtime errors. im not aware of better docstring parsers, but am happy to add a try/except around this

@nancydyc nancydyc added this to Needs Triage in Core via automation Dec 2, 2020
@yuhan yuhan added this to Untriaged in Practitioner via automation May 4, 2021
@yuhan yuhan moved this from Untriaged to High Priority in Practitioner May 4, 2021
@sryza
Copy link
Contributor

sryza commented May 28, 2021

@sryza sryza moved this from High Priority to In Progress in Practitioner Jun 1, 2021
sryza added a commit that referenced this issue Jun 3, 2021
Summary:
#3306

We could potentially emit a warning.

Test Plan: bk

Reviewers: alangenfeld, cdecarolis

Reviewed By: alangenfeld

Differential Revision: https://dagster.phacility.com/D8170
@sryza
Copy link
Contributor

sryza commented Jun 3, 2021

The fix will be included in next week's release

@sryza sryza closed this as completed Jun 7, 2021
Practitioner automation moved this from In Progress to Done Jun 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't working
Projects
No open projects
Core
Needs Triage
Development

No branches or pull requests

4 participants