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

Code generation and need for discriminator #36

Open
kwalcock opened this issue Oct 9, 2022 · 9 comments
Open

Code generation and need for discriminator #36

kwalcock opened this issue Oct 9, 2022 · 9 comments

Comments

@kwalcock
Copy link
Contributor

kwalcock commented Oct 9, 2022

Java/Scala code generation doesn't work from files like gromet_FN_v0.1.4.yaml and gromet_metadata_v0.1.4.yaml for lack of a discriminator in superclasses. For example,

    GrometObject:
      description: Generic base class for any GroMEt object
      type: object
      properties:
        metadata:
          description: Index (integer) into the metadata_collection table in GrometFNModule.
          type: integer
# Lines like the ones below need to be added     
        gromet_object_type: # added
          type: string #
      discriminator: # added
        propertyName: gromet_object_type

When subclass objects are written out, a field should then appear as such:

{
    "gromet_object_type": "GrometFNModule",
    "schema": "FN",
    "schema_version": "0.1.4",

This allows the generated Java/Scala code to figure out how to deserialize the object.

I don't (yet) know how the Python code is working without this, but will probably notice soon. I am hand editing the files like cond1--Gromet-FN-auto.json to add this information to make sure the updates to the yaml file that I'm about to suggest work.

Perhaps this issue has been encountered before, so I'm filing this slightly prematurely in case there's more I should know before continuing.

@kwalcock
Copy link
Contributor Author

@cl4yton, are you the one to ask about this?

@cl4yton
Copy link
Contributor

cl4yton commented Oct 11, 2022

Thanks @kwalcock for pointing this out, and yes, I'm among the folks to bug about this. I think this may have gone unnoticed by us so far. I'm adding @vincentraymond-ua and @titomeister to this thread.

@vincentraymond-ua: Can you take a look at this, given you familiarity with the importer?

@cl4yton
Copy link
Contributor

cl4yton commented Oct 11, 2022

Commenting again b/c I just realized I had not added @vincentraymond-ua to this project until just now...

@vincentraymond-ua
Copy link
Collaborator

vincentraymond-ua commented Oct 11, 2022

In the case of the Python importer, we can work around this since there is no ambiguity in the Gromet about what type of object we are dealing with. For example, the entries in "bf" will always be a GrometBoxFunction, the entries in "bc" will always be a GrometBoxConditional, and the entires in "bl" will always be a GrometBoxLoop.

So, we just use that knowledge to create the correct type. I can't think of or recall an example of a field where we can have more than one type of GrometObject, but I could be incorrect.

@titomeister @cl4yton - What do you think?

@vincentraymond-ua
Copy link
Collaborator

Also, I do see value in adding a discriminator for Gromet Objects, and we already have something similar for the superclass Metadata (metadata_type)

@cl4yton
Copy link
Contributor

cl4yton commented Oct 11, 2022

Hi @kwalcock @vincentraymond-ua @titomeister : I could use some discussion on this as I think I might not understand the issue. I just create an ASKEM PA meeting for tomorrow (Tito and Vincent: We can review progress on our task list), but also invited @kwalcock (if you are available) to join at the beginning to see if we can work out the issue.

@kwalcock
Copy link
Contributor Author

Yes, I can be at that meeting. I was surprised that the client generated for Java would not not process the files, even though it looks perfectly feasible. FWIW, the code I'm using is basically

new io.swagger.client.JSON().deserialize(text, classOf[GrometFNModule])

where text comes from files like gromet_FN_v0.1.4.yaml. It seems like the Java code generator is assuming that these types will be used polymorphically so that it will by necessity need to have a hint about which subclass is being encountered. It might also have to do with the gson being used under the hood. Whatever the reason, it seems like adding the discriminator works. In not too long I could know for certain.

I realize we're mostly using Python mostly, but guess that some other group might want to use the definitions in a different way. It looks like it wouldn't have worked out of the box.

@kwalcock
Copy link
Contributor Author

It looks like the program to work around some of the problems with having the model in two separate files is this:

https://github.com/ml4ai/automates/blob/master/scripts/swagger/codegen_swagger_models.py

I'm thinking that I can instead read the two yaml files, combine the component/schemas path of each and generate a single set of files. That's going to make life easier for me while editing the files, so I'll try it.

@kwalcock
Copy link
Contributor Author

Here are some notes about code generation:

I wrote programs to generate code via either of the libraries

    "io.swagger.codegen.v3" % "swagger-codegen-cli"   % "3.0.35",
    "org.openapitools"      % "openapi-generator-cli" % "6.2.0",

from https://github.com/swagger-api/swagger-codegen and https://github.com/OpenAPITools/openapi-generator and then tried to read in the json documents that had already produced. For the swagger library, neither the code generated for java nor scala was able to read in the files, even with a substantial amount of rewriting the generated code. For the openapitools, none of generators for scalatra, scala-akka, scala-finch, scala-logom-server, scala-play-server, scala-sttp, or scalaz produced hopeful results. They weren't exactly hopeless, but it was taking significantly longer to figure out what was wrong with them or how to use them than it would take to translate the spec by hand, at least if the conversion only needed to be done once. I did do that translation and it reads in all the files. It's right now in https://github.com/kwalcock/skema and specifically at https://github.com/kwalcock/skema/tree/main/gromet/model-scala-v0.1.4/src/main/scala/org/ml4ai/skema/gromet/model/scala/v0_1_4.

I would not say "To generate Java classes rather, change the -l python to -l java." Code may be generated, but it isn't usable.

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

No branches or pull requests

3 participants