Skip to content

Move nonbillable pydantic models from invoicing-private-data#291

Open
marcoagonzales007 wants to merge 4 commits into
CCI-MOC:mainfrom
marcoagonzales007:nonbillable-pydantic-models
Open

Move nonbillable pydantic models from invoicing-private-data#291
marcoagonzales007 wants to merge 4 commits into
CCI-MOC:mainfrom
marcoagonzales007:nonbillable-pydantic-models

Conversation

@marcoagonzales007

@marcoagonzales007 marcoagonzales007 commented Apr 17, 2026

Copy link
Copy Markdown
Contributor

-Copied the pydantic models from invoicing-private-data/validate_projects/models.py into a new process_report/nonbillable_models.py in this repo.
-Updated loader.py to validate the YAML through the pydantic models instead of using raw dicts
Closes #287

@QuanMPhm QuanMPhm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A few structural comments

Comment thread process_report/loader.py Outdated
Comment on lines +158 to +166
data = yaml.safe_load(file)
projects = ExcludedProjectList.model_validate(data)
for project in projects.root:
project_name = project.name
cluster_list = project.clusters.root
is_billable = project.is_billable

if project.start:
if not _is_in_time_range(project.start, project.end):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you move all the parsing logic here into the ExcludedProjectList models? Same goes for the PI-list parsing logic as well. They should have a function such that given the invoice month, returns both the list of timed, and non-timed projects. It makes the most sense for the models to have the parsing/validation logic, and for loader.py to only handle "loading".

Afterwards, can you move the test cases for the models over here as well? Place them in a unit test file

Comment thread pyproject.toml Outdated

@QuanMPhm QuanMPhm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking better. A few comments

Comment thread process_report/models/cluster_names.txt Outdated
Comment thread process_report/models/nonbillable_models.py Outdated
Comment thread process_report/models/su_types.txt Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Follow-up on the comment about some functions that should be moved in the pydantic models themselves, this file should now contain test cases for the nonbillable list that has been sitting in test_util.py.

When you move the test cases over here, you should be able to use pytest.parametrize to help you create test data for the nonbillable list, and pytest-mock to mock get_allowed_clusters()

Comment thread process_report/nonbillable_models.py Outdated
Comment thread pyproject.toml Outdated
@marcoagonzales007 marcoagonzales007 force-pushed the nonbillable-pydantic-models branch from 7a58604 to 422cd01 Compare May 9, 2026 19:26

@QuanMPhm QuanMPhm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just two more comments

Comment thread process_report/tests/unit/test_nonbillable_models.py Outdated
Comment thread process_report/loader.py Outdated
Comment on lines 117 to 119
def _load_pi_config(self, filepath: str) -> PIList:
with open(filepath) as file:
pi_list = yaml.safe_load(file)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since _load_pi_config will only ever receive invoice_settings.nonbillable_pis_filepath as the file to load, could you do this instead?

def _load_pi_config(self) -> PIList:
        with open(filepath) as file:
            pi_list = yaml.safe_load(invoice_settings.nonbillable_pis_filepath)

        return PIList.model_validate(pi_list)

    def get_nonbillable_pis(self) -> list[str]:
        return self._load_pi_config().get_nonbillable_pis()

    def get_pi_non_billed_su_types(self) -> dict[str, list[str]]:
        """PI usernames -> list of SU types that receive credit (zeroed out)."""
        eturn self._load_pi_config().get_pi_non_billed_su_types()

@QuanMPhm QuanMPhm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The code changes looks good. Can you squash all your commits into one, and write a concise commit message?

@marcoagonzales007 marcoagonzales007 force-pushed the nonbillable-pydantic-models branch from 107f3fe to 79190e1 Compare May 23, 2026 20:33

@QuanMPhm QuanMPhm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One minor comment. Otherwise, this looks good. @knikolla @naved001 I believe this is good enough for your review

Comment thread requirements.txt Outdated
validators
python-dateutil
pydantic-settings
pytest-mock

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I just realized, can you put this into the test-requirement.txt file, as this is a dependancy for tests?

@QuanMPhm QuanMPhm requested review from knikolla and naved001 June 4, 2026 13:22
@marcoagonzales007 marcoagonzales007 force-pushed the nonbillable-pydantic-models branch from 79190e1 to f2bdb8d Compare June 11, 2026 17:00

@QuanMPhm QuanMPhm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems the recently merged PR introduced some merge conflicts. Please resolve them

@QuanMPhm QuanMPhm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good!

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.

Move validation code from invoicing-private-data to here

2 participants