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

fix crash trying to extend None list member #42

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

wakamex
Copy link

@wakamex wakamex commented Apr 1, 2023

attempting py2puml elfpy elfpy > elfpy.plantuml on https://github.com/element-fi/elf-simulations/tree/2349ebf365f5c3c589ad42d85a263a241010dac7

Traceback (most recent call last):
  File "/Users/mihai/.pyenv/versions/py2puml/bin/py2puml", line 8, in <module>
    sys.exit(run())
  File "/Users/mihai/.pyenv/versions/3.10.10/envs/py2puml/lib/python3.10/site-packages/py2puml/cli.py", line 24, in run
    print(''.join(py2puml(args.path, args.module)))
  File "/Users/mihai/.pyenv/versions/3.10.10/envs/py2puml/lib/python3.10/site-packages/py2puml/py2puml.py", line 12, in py2puml
    inspect_package(
  File "/Users/mihai/.pyenv/versions/3.10.10/envs/py2puml/lib/python3.10/site-packages/py2puml/inspection/inspectpackage.py", line 20, in inspect_package
    inspect_module(
  File "/Users/mihai/.pyenv/versions/3.10.10/envs/py2puml/lib/python3.10/site-packages/py2puml/inspection/inspectmodule.py", line 60, in inspect_module
    inspect_domain_definition(definition_type, root_module_name, domain_items_by_fqn, domain_relations)
  File "/Users/mihai/.pyenv/versions/3.10.10/envs/py2puml/lib/python3.10/site-packages/py2puml/inspection/inspectmodule.py", line 47, in inspect_domain_definition
    inspect_class_type(
  File "/Users/mihai/.pyenv/versions/3.10.10/envs/py2puml/lib/python3.10/site-packages/py2puml/inspection/inspectclass.py", line 103, in inspect_class_type
    instance_attributes, compositions = parse_class_constructor(class_type, class_type_fqn, root_module_name)
  File "/Users/mihai/.pyenv/versions/3.10.10/envs/py2puml/lib/python3.10/site-packages/py2puml/parsing/parseclassconstructor.py", line 42, in parse_class_constructor
    visitor.visit(constructor_ast)
  File "/Users/mihai/.pyenv/versions/3.10.10/lib/python3.10/ast.py", line 418, in visit
    return visitor(node)
  File "/Users/mihai/.pyenv/versions/3.10.10/envs/py2puml/lib/python3.10/site-packages/py2puml/parsing/astvisitors.py", line 103, in generic_visit
    NodeVisitor.generic_visit(self, node)
  File "/Users/mihai/.pyenv/versions/3.10.10/lib/python3.10/ast.py", line 426, in generic_visit
    self.visit(item)
  File "/Users/mihai/.pyenv/versions/3.10.10/lib/python3.10/ast.py", line 418, in visit
    return visitor(node)
  File "/Users/mihai/.pyenv/versions/3.10.10/envs/py2puml/lib/python3.10/site-packages/py2puml/parsing/astvisitors.py", line 113, in visit_FunctionDef
    self.generic_visit(node)
  File "/Users/mihai/.pyenv/versions/3.10.10/envs/py2puml/lib/python3.10/site-packages/py2puml/parsing/astvisitors.py", line 103, in generic_visit
    NodeVisitor.generic_visit(self, node)
  File "/Users/mihai/.pyenv/versions/3.10.10/lib/python3.10/ast.py", line 426, in generic_visit
    self.visit(item)
  File "/Users/mihai/.pyenv/versions/3.10.10/lib/python3.10/ast.py", line 418, in visit
    return visitor(node)
  File "/Users/mihai/.pyenv/versions/3.10.10/envs/py2puml/lib/python3.10/site-packages/py2puml/parsing/astvisitors.py", line 150, in visit_Assign
    self.extend_relations(full_namespaced_definitions)
  File "/Users/mihai/.pyenv/versions/3.10.10/envs/py2puml/lib/python3.10/site-packages/py2puml/parsing/astvisitors.py", line 85, in extend_relations
    self.uml_relations_by_target_fqn.update({
  File "/Users/mihai/.pyenv/versions/3.10.10/envs/py2puml/lib/python3.10/site-packages/py2puml/parsing/astvisitors.py", line 88, in <dictcomp>
    if target_fqn.startswith(self.root_fqn) and (
AttributeError: 'NoneType' object has no attribute 'startswith'

@lucsorel
Copy link
Owner

lucsorel commented Apr 3, 2023

hello @wakamex, thank you for reporting the issue.

I should first investigate why target_fqn is None, this should not happen. I don't have much time to investigate the issue this week, I need some time. Forgive me in advance to keep you waiting 🙏

@wakamex
Copy link
Author

wakamex commented Apr 3, 2023

the cause is assignments in base.py: https://github.com/element-fi/elf-simulations/blob/2349ebf365f5c3c589ad42d85a263a241010dac7/elfpy/markets/base.py#L108-L109

        self.market_state = market_state
        self.block_time = block_time

replacing our init function with the following also solves the problem (without requiring a tweak to py2puml):

    pricing_model: PricingModel
    market_state: State
    block_time: time.BlockTime

    def __init__(self, **kwargs):
        for key, value in kwargs.items():
            setattr(self, key, value)

along with moving theimport elfpy.time as time import outside of the if TYPE_CHECKING condition (diff delvtech/agent0@f8bc813)

I'm not sure why this, but I suspect it's because we're doing something totally-not-good with our type hints

hopefully you understand this better than I do 😄

I added a commit here with the print statements I used to arrive at this conclusion

@wakamex wakamex marked this pull request as draft April 3, 2023 18:52
@wakamex
Copy link
Author

wakamex commented Apr 3, 2023

the diff between the two resulting plantuml's delvtech/agent0@f22a7de

@lucsorel
Copy link
Owner

lucsorel commented Apr 4, 2023

thanks for investigating the issue 🙏

The main reason of the issue seems this:

moving the import elfpy.time as time import outside of the if TYPE_CHECKING condition

Indeed, py2puml combines 2 techniques when handling native Python classes:

  • it parses the abstract syntax tree (the source code) of the __init__ constructors to detect the self.attr = attr assignments, looking for the type annotation of attr (in the constructor signature) to affect it to self.attr in the PlantUML diagram
  • it imports the module of the class to understand the type of self.attr in order to draw relationships between your classes in the UML diagram

In you case, the type associated to block_time is not defined at the module level (in an import done at the top of the file) but dynamically when some code executes. Therefore, py2puml cannot assign a type to self.block_time. Dynamically importing modules is not a recommended practice and I didn't know that some projects actually do that (and there may be some good reasons to do it; I am not judging, I am only saying that it is a practice py2puml does not handle).

If you can, I would recommend to move such dynamic imports at the top of the module and restore your original constructor (with self.attr = attr assignments) so that py2puml can yield the proper class diagram: these properties are instance attributes, not class attributes.

I should definitely think of a safety net for these cases, such as skipping the potential relationship between this class and the attribute's class. Maybe by introducing a default fail-fast mode, and an optional fail-proof mode (producing a degraded class diagram).

Side remark: please, follow the contribution guidelines about the use of quotes and the formatting as described in https://github.com/lucsorel/py2puml/blob/main/CONTRIBUTING.md#code-styling 🙏 Your IDE settings seems to reformat all the files you modified and replace single quotes with double quotes, making the codebase inconsistent and hard for me to read. And it makes the pull-request harder to understand.

I realize that I should add pre-commit hooks to the project in order to avoid these troubles now that several people are eager to contribute (and thanks again for reporting the issue) 😃

@lucsorel
Copy link
Owner

hi @wakamex 😃

Did you manage to fix the crash by placing the import at the top of the Python file (without modifying the py2puml code)?

@lucsorel lucsorel added the waiting for feedback questions waiting to be answered label Apr 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting for feedback questions waiting to be answered
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants