chore: remove pydantic validation from dto classes #740
Conversation
src/libecalc/domain/infrastructure/energy_components/asset/asset.py
Outdated
Show resolved
Hide resolved
| @@ -288,6 +292,10 @@ def evaluate_rate_ps_pd_density( | |||
| :param discharge_pressures: | |||
| :param fluid_density: | |||
| """ | |||
| # Ensure rate is a NumPy array | |||
| rate = np.array(rate, dtype=np.float64) | |||
| fluid_density = np.array(fluid_density, dtype=np.float64) | |||
There was a problem hiding this comment.
Maybe we should consider using asarray or asfarray? asarray won't copy when the input already is an array. asfarray will create a floating point ndarray, I'm not sure what that actually is array with dtype=float or something else.
|
|
||
|
|
||
| class Asset(Component, EnergyComponent): | ||
| def __init__( |
There was a problem hiding this comment.
Component is no longer necessary, but we would have to make sure not to use component_type directly when removing it. EnergyComponent has the same information as Component.
| self.generator_set_model = generator_set_model | ||
| self.consumers = consumers | ||
| self.cable_loss = cable_loss | ||
| self.max_usage_from_shore = max_usage_from_shore |
There was a problem hiding this comment.
Can we move the generator specific stuff to generator set?
| @@ -46,61 +43,96 @@ def check_regularity(cls, regularity): | |||
|
|
|||
|
|
|||
| class BaseEquipment(BaseComponent, ABC): | |||
| user_defined_category: dict[Period, ConsumerUserDefinedCategoryType] = Field(..., validate_default=True) | |||
| def __init__( | |||
There was a problem hiding this comment.
I think we should look into removing BaseEquipment and BaseComponent. Instead we could create objects that contains several of these values if we think it makes sense to group them. Or just duplicate the init in each energy component.
| return user_defined_category | ||
|
|
||
|
|
||
| class BaseConsumer(BaseEquipment, ABC): | ||
| """Base class for all consumers.""" | ||
|
|
||
| consumes: ConsumptionType | ||
| fuel: Optional[dict[Period, FuelType]] = None | ||
| def __init__( |
There was a problem hiding this comment.
Also remove BaseConsumer I think. We don't want to use inheritance without being sure the abstraction makes sense, here it's just used to avoid having to specify the same params.
| @@ -226,6 +228,7 @@ def from_yaml_to_dto( | |||
| user_defined_category=user_defined_category, | |||
| cable_loss=cable_loss, | |||
| max_usage_from_shore=max_usage_from_shore, | |||
| component_type=ComponentType.GENERATOR_SET, # TODO: Check if this is correct. Why isn´t component_type set for YamlGeneratorSet? | |||
There was a problem hiding this comment.
Do you mean why data.component_type can't be used instead? We don't have types here in the yaml, it's based on context, i.e. the key under which this data is put makes it a generator set.
Instead of specifying component_type here, we could just hardcode it in the constructor though. The reason we don't do that already was pydantic related I think. component_type and class type means the same in this case.
| @@ -114,7 +135,7 @@ def to_dto( | |||
| regularity=regularity, | |||
| consumes=consumes, | |||
| component_conditions=component_conditions, | |||
| stream_conditions_priorities=TypeAdapter(YamlPriorities).dump_python( | |||
| stream_conditions_priorities=self.convert_yaml_priorities( | |||
There was a problem hiding this comment.
We should probably map this similar to v1/the other components, in the mapper (removing to_dto). But we'll see what we end up doing with v2, seems unnecessary to maintain if we have no plan to make users migrate.
| @@ -13,7 +13,7 @@ | |||
|
|
|||
| class TestElectricityConsumer: | |||
| def test_invalid_energy_usage(self): | |||
| with pytest.raises(ValidationError) as e: | |||
| with pytest.raises(ValueError) as e: | |||
There was a problem hiding this comment.
This change is probably something we need to handle. Previously pydantic would collect all value errors and create a validation error with everything (making the webapp display all errors). Now we probably don't even handle the single ValueError correctly? Or I might have missed a change in this PR, but we should still decide what to do with validation errors. Collecting several ValueErrors is probably a good idea?
equinor/ecalc-internal#291
Why is this pull request needed?
Pydantic validation is taken care of in the Yaml classes now. It is not needed in the dto classes, here we need more flexibility.
What does this pull request change?
Remove inheritance from EcalcBaseModel (Pydantic) in Component base class. Remove: field_validators, Field definitions, ConfigDict import and usage. Remove Pydantic specific imports and replace with standard Python types if necessary. In libecalc/domain/infracstructure/energy_components/ this is done for: