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

Date to datetime #245

Closed
wants to merge 4 commits into from
Closed

Conversation

vladistan
Copy link
Collaborator

First attempt at using datetime instead of dates

@vladistan vladistan marked this pull request as draft September 11, 2024 16:27
@debajyotid2 debajyotid2 marked this pull request as ready for review September 11, 2024 22:13
@debajyotid2
Copy link
Collaborator

@vladistan @AdamFinkle Pytest appears to be failing

ERROR tests/test_rules_engine/test_engine.py::test_convert_to_intermediate_billing_periods - KeyError: 'analysis_type_override'
ERROR tests/test_rules_engine/test_engine.py::test_get_outputs_normalized - KeyError: 'analysis_type_override'

because of the way normalized billing period instances are initialized.

        # billing_periods = [
        #     NormalizedBillingPeriodRecordBase(**x) for x in billing_periods_dict
        # ]
    
>       billing_periods = [
            NormalizedBillingPeriodRecordBase(
                period_start_date=date.fromisoformat(x["period_start_date"]),
                period_end_date=date.fromisoformat(x["period_end_date"]),
                usage=x["usage"],
                analysis_type_override=x["analysis_type_override"],
                inclusion_override=x["inclusion_override"],
            ) for x in billing_periods_dict
        ]

versus

    billing_periods = [
        NormalizedBillingPeriodRecordBase(**x) for x in billing_periods_dict
    ]

@debajyotid2 debajyotid2 marked this pull request as draft September 11, 2024 22:34
@vladistan
Copy link
Collaborator Author

Ahh, you enabled the workflow. Let us examine what's going on. We run all the tests in our codespace before creating the PR and all the tests were passing. Something is different.

FYI, the reason we changed the way that NormalizedBillingPeriodRecordBase are created is because pydantic was choking on your test data. Now that we have datetime instead of date, pydantic expects start_date and end_date to be in ISO timestamp format which is '2024-10-09T05:23:11

when you explicitly instantiate the objects with the constructor instead of using **x you can supply your own parsers for each field.

let us dig deeper into this

@vladistan vladistan marked this pull request as ready for review September 12, 2024 17:26
@vladistan
Copy link
Collaborator Author

@debajyotid2 @thadk I put a few fixes, could you approve the workflow to see how it goes?

@debajyotid2
Copy link
Collaborator

debajyotid2 commented Sep 12, 2024

Nice - the rules engine unit tests are passing!

The pyodide tests still fail because the dates are not being converted into datetime objects while parsing. This is because in TemperatureInput we do not have a parser defined to convert date strings to datetime, and we cannot instantiate TemperatureInput objects by directly calling the constructor. We'll still need to change the way date strings are being parsed in pyodide.test.ts. @thadk do you think we need to fix this before merging?

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.

2 participants