PrefectHQ / prefect

Prefect is a workflow orchestration framework for building resilient data pipelines in Python.
https://prefect.io
Apache License 2.0
15.66k stars 1.53k forks source link

Enhanced deployment parameters UI does not support null values #12066

Closed samdchamberlain closed 5 months ago

samdchamberlain commented 6 months ago

First check

Bug summary

For the new enhanced deployment parameters UI, the validation checks are failing for null or None values despite correctly displaying Optional for those parameters. This makes the updated deployment parameters UI unusable

Screenshot 2024-02-22 at 5 07 51 PM

Reproduction

See example here%255Cn)%255Cn%255CnSELECT%255Cn%2520%2520id%255Cn%2520%2520%252C%2520created_at%255Cn%2520%2520%252C%2520target%255CnFROM%2520data%255CnWHERE%2520id%2520is%2520not%2520null%255Cn%2522%252C%2522key_column%2522%253A%2522ID%2522%252C%2522target_column%2522%253A%2522TARGET%2522%252C%2522sql_query_params%2522%253A%257B%257D%257D%252C%2522key_column%2522%253A%2522ID%2522%252C%2522data_splits%2522%253A%257B%2522date_column%2522%253A%2522CREATED_AT%2522%252C%2522sample_size%2522%253A%257B%2522test%2522%253A%257B%2522total%2522%253A100000%252C%2522negative_class%2522%253Anull%252C%2522positive_class%2522%253Anull%257D%252C%2522train%2522%253A%257B%2522total%2522%253Anull%252C%2522negative_class%2522%253A1000000%252C%2522positive_class%2522%253A1000000%257D%257D%252C%2522days_ago_bounds%2522%253A%257B%2522test%2522%253A%257B%2522end%2522%253A23%252C%2522start%2522%253A30%257D%252C%2522train%2522%253A%257B%2522end%2522%253A31%252C%2522start%2522%253A150%257D%257D%252C%2522start_end_dates%2522%253Anull%257D%252C%2522prefect_env%2522%253A%2522preprod%2522%252C%2522container_name%2522%253A%2522payments%2522%252C%2522hpt_parameters%2522%253A%257B%2522num_rounds%2522%253A10%252C%2522sample_frac%2522%253A0.1%252C%2522search_space%2522%253A%257B%2522max_depth%2522%253A%255B1%252C20%255D%252C%2522subsample%2522%253A%255B0.1%252C1%255D%252C%2522n_estimators%2522%253A%255B100%252C400%255D%252C%2522colsample_bytree%2522%253A%255B0.00001%252C1%255D%252C%2522scale_pos_weight%2522%253A%255B1%252C5%255D%257D%252C%2522fixed_parameters%2522%253A%257B%2522eta%2522%253A0.17%252C%2522lambda%2522%253A1%252C%2522n_jobs%2522%253A96%252C%2522eval_metric%2522%253A%2522map%2522%252C%2522min_child_weight%2522%253A5%252C%2522early_stopping_rounds%2522%253A20%257D%257D%252C%2522attendant_config%2522%253A%257B%2522owner%2522%253A%2522schamberlain%2522%252C%2522export_to_mux%2522%253Atrue%252C%2522target_backend%2522%253A%2522onnx%2522%252C%2522deployment_destination%2522%253A%2522Risk%2522%257D%252C%2522model_parameters%2522%253A%257B%2522eta%2522%253A0.17%252C%2522lambda%2522%253A1%252C%2522max_depth%2522%253A5%252C%2522num_round%2522%253A300%252C%2522subsample%2522%253A0.7%252C%2522colsample_bytree%2522%253A0.7%252C%2522min_child_weight%2522%253A5%252C%2522scale_pos_weight%2522%253A1%252C%2522num_parallel_tree%2522%253A8%257D%252C%2522columns_to_exclude%2522%253A%255B%2522CREATED_AT%2522%255D%252C%2522in_memory_training%2522%253Afalse%252C%2522pipeline_yaml_path%2522%253A%2522chargebacks_xgb%252Flabels%252Fpayments%252Fgeneral_chargebacks.yaml%2522%252C%2522final_training_sets%2522%253A%255B%2522train%2522%252C%2522test%2522%255D%252C%2522upload_to_attendant%2522%253Atrue%257D%252C%2522prefect_flow_name%2522%253A%2522ctw_payments_general_chargebacks%2522%257D)

Error

Screenshot 2024-02-22 at 5 07 51 PM

Browsers

Prefect version

No response

Additional context

No response

pleek91 commented 6 months ago

@samdchamberlain I believe if you just omit the values altogether it'll pass validation.

Would you expect a property that is type int to also accept None? If your property was total: int = None or total: Union[int, None] then it would accept None as an actual value. But since its just int it either wants the value to be an int or for no value to be passed.

If you could post an example flow I can test it out too. I don't have access to your workspace.

Also, There's a checkbox at the bottom of the form that says "Validate parameters before submitting". If you uncheck that you can skip validation and just submit the run.

samdchamberlain commented 5 months ago

@samdchamberlain I believe if you just omit the values altogether it'll pass validation.

Would you expect a property that is type int to also accept None? If your property was total: int = None or total: Union[int, None] then it would accept None as an actual value. But since its just int it either wants the value to be an int or for no value to be passed.

If you could post an example flow I can test it out too. I don't have access to your workspace.

Also, There's a checkbox at the bottom of the form that says "Validate parameters before submitting". If you uncheck that you can skip validation and just submit the run.

@pleek91 The property as setup in our code is total: Union[int, None] or total: Optional[int], so we did set it up as you suggested and still hit this error. The way in which Prefect appears to be inferring the property is losing track of this information. We can try the skip validation option, but we have setup our data properties as you suggest and Prefect is enforcing an int data property that isn't being set by the end user

I'm not able to give access to the workspace but we can try to sync with our contacts at Prefect on this to look into the example in more detail, but this does appear to be a bug based on your explanation with respect to how our example is structured

pleek91 commented 5 months ago

@samdchamberlain I think what we're seeing here is a difference between pydantic v1 and pydantic v2. We're using pydantic to create json schemas and then we use those schemas to generate forms and also do validation. But there is a bug in pydantic v1 that causes None to not show up in the schema. So since we're validating against the schema it thinks its not valid.

We're investigating to see if we can find a solution on our side to account for the pydantic bug. In the mean time some options are

I'll report back when we finish our investigation. Hopefully we'll be able to account for the pydantic bug and make this work without any changes on your part.

samdchamberlain commented 5 months ago

@samdchamberlain I think what we're seeing here is a difference between pydantic v1 and pydantic v2. We're using pydantic to create json schemas and then we use those schemas to generate forms and also do validation. But there is a bug in pydantic v1 that causes None to not show up in the schema. So since we're validating against the schema it thinks its not valid.

We're investigating to see if we can find a solution on our side to account for the pydantic bug. In the mean time some options are

  • Uncheck the "Validate parameters before submitting" box before submitting a run (or uncheck it after you're only left with this specific validation error)
  • There's a hack mentioned in a comment on the pydantic github issue that might be useful
  • The bug was fixed in pydantic v2. So if you were able to upgrade pydantic and redeploy that would fix it as well

I'll report back when we finish our investigation. Hopefully we'll be able to account for the pydantic bug and make this work without any changes on your part.

Great! Thanks so much for the update. We are able to make it work with unchecking "Validate parameters before submitting". When the pydantic version gets updated we will try it again as well and see how that looks

pleek91 commented 5 months ago

@samdchamberlain we were able to do some tricks on the validation side so that if you do pass null for this type of schema it should pass validation. Can you let me know how this is working out for you?

pleek91 commented 5 months ago

I'm going to close this issue for housekeeping. But please let us know if you're still having issues with this.