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

[Bug]: Extensions are not robust to order #1086

Open
rly opened this issue Mar 27, 2024 · 6 comments
Open

[Bug]: Extensions are not robust to order #1086

rly opened this issue Mar 27, 2024 · 6 comments
Assignees
Labels
category: bug errors in the code or code behavior priority: low alternative solution already working and/or relevant to only specific user(s)

Comments

@rly
Copy link
Contributor

rly commented Mar 27, 2024

What happened?

If an extension defines data type A that includes or links to data type B and data type A is defined before data type B, then resolution fails. This issue can be worked around by re-ordering the data type definitions so that B is defined before A in the YAML.

While we could make a rule that says a data type must be defined before it is referenced in another data type, that is not very user-friendly behavior. On read of a YAML file, I think we could scan for includes and links and resolve the types in a smart order that avoids these issues if possible.

Circular references would still be an issue, but that is rare.

This ordering issue can also happen across multiple YAML files, but it is very rare to have multiple YAML files, so I think improving that is extra-low priority.

Steps to Reproduce

See https://github.com/NeurodataWithoutBorders/pynwb/issues/1873

Traceback

No response

Operating System

macOS

Python Executable

Conda

Python Version

3.12

Package Versions

No response

@rly rly added category: bug errors in the code or code behavior priority: low alternative solution already working and/or relevant to only specific user(s) labels Mar 27, 2024
@rly
Copy link
Contributor Author

rly commented Mar 27, 2024

@mavaylon1 do you think you could take this on?

@oruebel
Copy link
Contributor

oruebel commented Mar 27, 2024

Just an observation. I think solving this in the most general case is going to be very tricky. I would hence suggest adding a rule/best practice in the extension docs on nwb-overview and also in the schema language docs to document that types should be defined before use. In compiled languages, like C++, types have to be at least declared before use (even if the definition comes later). If addressing the common case is easy, then let's do that, but I think this issue is likely a rabbit whole, so we probably want to limit the kind of cases we address vs. making it rule. Maybe better reporting of errors to indicate when a type is being used before it has been defined could go a long way to help.

@mavaylon1
Copy link
Contributor

@rly I can, but I'll take a look in two-three weeks due to current PRs and the roadshow. Is that timeline okay with you?

@rly
Copy link
Contributor Author

rly commented Mar 27, 2024

@mavaylon1 Yes, this is low-priority and not that important. If you could in a few weeks, please spend 10 min looking into how hard it would be to do this. I think it might be useful for you to learn how extension specs are resolved. But I'd say if you don't think you can do it relatively quickly (<2 hours), then let's table it and instead just update the error to say something like "Please ensure that data type X is defined in the schema YAML file before being used in data type Y" and update the docs.

@mavaylon1
Copy link
Contributor

@rly I will take a look after dev days

@mavaylon1 mavaylon1 self-assigned this Jul 25, 2024
@mavaylon1
Copy link
Contributor

@rly This got lost in backlog of items. Let's sync on this in next few weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: bug errors in the code or code behavior priority: low alternative solution already working and/or relevant to only specific user(s)
Projects
None yet
Development

No branches or pull requests

3 participants