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: object_default/schema #677

Merged
merged 5 commits into from
Mar 26, 2024
Merged

Fix: object_default/schema #677

merged 5 commits into from
Mar 26, 2024

Conversation

younesdessia
Copy link
Contributor

  • What kind of changes does this PR introduce?
  • Bug fix
  • new features
  • performance
  • docs update
  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
  • Yes:
  • No
  • Other information:

@younesdessia younesdessia added the Priority: High Issue has a high dev/value ratio label Feb 28, 2024
Copy link
Collaborator

@GhislainJ GhislainJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need more context about this, as None as default value should directly happen with objects. We should normally have an Optional schema

@younesdessia
Copy link
Contributor Author

@GhislainJ
here is a case of reproduction

from typing import Optional
from dessia_common.core import DessiaObject


class ObjectTest(DessiaObject):

    def __init__(self, name: str = "", measure: float = 1.1):
        self.measure = measure
        DessiaObject.__init__(self, name=name)


class InstantiateObject(DessiaObject):

    def __init__(self, name: str = ""):
        DessiaObject.__init__(self, name=name)

    def compute(self, my_object: Optional[ObjectTest] = None):
        return 404
from dessia_common.typings import MethodType
from dessia_common.workflow.blocks import InstantiateModel, ModelMethod
from dessia_common.workflow.core import Pipe, Workflow
from navalgroup_custom.dev_dc import InstantiateObject

block_0 = InstantiateModel(model_class=InstantiateObject, name="InstantateObject")
block_1 = ModelMethod(method_type=MethodType(InstantiateObject, 'compute'), name="compute")
blocks = [block_0, block_1]


pipe_1 = Pipe(block_0.outputs[0], block_1.inputs[0])

workflow = Workflow(blocks, [pipe_1], output=block_1.outputs[0], name="test")

@younesdessia
Copy link
Contributor Author

Hi @GhislainJ

I checked with debug mode, for me there is no other problem except in the method object_default, and the fix is correct.

to reprduce the issue you can just do this:
workflow.method_schemas

@younesdessia younesdessia added the Status: Ready for review PR is ready to be reviewed. Should pass CI label Mar 8, 2024
@younesdessia
Copy link
Contributor Author

Another fix, replace None value of default_value with UNDEFINED in SchemaAttribute class:

image

@GhislainJ
Copy link
Collaborator

Approved but adding tests are needed to merge

@GhislainJ GhislainJ added Status: Commented Draw attention to comments written by the dev team and removed Status: Ready for review PR is ready to be reviewed. Should pass CI labels Mar 26, 2024
Copy link
Collaborator

@GhislainJ GhislainJ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests using you simplified repro case should be added

@younesdessia
Copy link
Contributor Author

Done !

@GhislainJ GhislainJ added Status: Ready for merge PR is ready to be merged. Should pass CI and removed Status: Commented Draw attention to comments written by the dev team labels Mar 26, 2024
@GhislainJ GhislainJ merged commit f005633 into master Mar 26, 2024
2 checks passed
@GhislainJ GhislainJ deleted the fix/check_schema branch March 26, 2024 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: High Issue has a high dev/value ratio Status: Ready for merge PR is ready to be merged. Should pass CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants