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 Unknown Item Type Specification #656

Merged
merged 18 commits into from
Mar 12, 2024

Conversation

garrettmflynn
Copy link
Member

@garrettmflynn garrettmflynn commented Mar 11, 2024

fix #652

This happened because there is no explicit item type for this property, and the default method was deriving the type from the first item—which didn't exist and therefore was undefined.

If no initial value exists to check, we are now assuming the array is an array of strings.

@garrettmflynn garrettmflynn self-assigned this Mar 11, 2024
@CodyCBakerPhD
Copy link
Collaborator

I remember this issue now

It actually stems from our method signature inference on annotation typing in NeuroConv not supported nested values like list[str]

When we implement the Pydantic method over there this should all be taken care of and we can do a big update of all such options

Though we could technically define it now by hard-coding it into the get_source_schema for that interface

we are now assuming the array is an array of strings.

That might be incorrect, though. Could you instead have the modal print a message along the lines of 'unknown schema type for interface option' or else detect it ahead of time and not display the modal?

@garrettmflynn
Copy link
Member Author

I'll define a hook that lets you specify custom behavior for when the input type has not been recognized from the schema (which is currently to print out the default value or, if nonexistent, indicate that there is no default value).

In this case, you'd rather have the property omitted than guess the structure of an improper JSON Schema?

@CodyCBakerPhD
Copy link
Collaborator

In this case, you'd rather have the property omitted than guess the structure of an improper JSON Schema?

Yeah. It could just as easily be a list of integers, or list of floats, etc. for any given interface

Plus the real trick of some of these (which pydantic will not completely solve for us either) is that their true proper schema isn't known until we read a little but of the file (i.e., need to know the source path first then determine the rest)

@garrettmflynn
Copy link
Member Author

Ah sure. How's this?
Screenshot 2024-03-11 at 12 59 35 PM

@CodyCBakerPhD
Copy link
Collaborator

Looks fine, broader question is if we want to display this kind of stuff or hide it away from the user. It might be a little while until this gets entirely fixed

@garrettmflynn
Copy link
Member Author

That's fair. I can define a way to hide these errored properties in production, but show the above message in Development.

Would this be too complicated to keep track of? Or d'you think this could be a good long-term solution?

@bendichter
Copy link
Collaborator

@bendichter
Copy link
Collaborator

I don't really want to print errors for failed rendering of optional arguments. This is really non-essential for conversion and we should not concern a user with its failure to render, but it would be helpful to know this internally so we can keep it on our to do list. I like the idea of displaying this in dev but not in prod, I think that addresses this nicely.

@CodyCBakerPhD
Copy link
Collaborator

Fine with me as well

@garrettmflynn
Copy link
Member Author

This is hidden in production now!

@CodyCBakerPhD CodyCBakerPhD enabled auto-merge (squash) March 12, 2024 16:32
@CodyCBakerPhD
Copy link
Collaborator

Looks like this breaks the main argument of the Audio interface

@garrettmflynn Can you push a fix to the schema for the Audio interface? Fine to override get_source_schema with hardcoded translation of a 'list of strings' for now

@CodyCBakerPhD
Copy link
Collaborator

Also what environment does the storybook use to generate snapshots? It seems like it's not using the proper version of NeuroConv for the Phy rendering

@garrettmflynn
Copy link
Member Author

Storybook uses the generated schema from generateInterfaceSchema.py, so we'll need to run that to update any of the schema aspects.

@CodyCBakerPhD CodyCBakerPhD merged commit 54791e9 into main Mar 12, 2024
15 checks passed
@CodyCBakerPhD CodyCBakerPhD deleted the fix-unknown-item-type-specification branch March 12, 2024 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Phy init Exclude Cluster Groups modal window is broken
3 participants