Skip to content

Conversation

@HansVRP
Copy link
Contributor

@HansVRP HansVRP commented Oct 3, 2025

first proposal to seperate the job database/dataframe logic and the processbasedjobstart

@HansVRP HansVRP requested a review from soxofaan October 3, 2025 12:42
@HansVRP
Copy link
Contributor Author

HansVRP commented Oct 7, 2025

TODO; move also the unit tests once settled on rearangement

@soxofaan
Copy link
Member

I'd try to do this exercise in atomic commits/PRs:

  • avoid moving unrelated things at the same time (like JobDatabaseInterface and ProcessBasedJobCreator here)
  • move related things in same commit: e.g. if you move ProcessBasedJobCreator, also move tests and update docs in same commit (or at least same PR)

that will avoid long-running PR review cycles that block other progress

e.g. I just started #824 to only move ProcessBasedJobCreator

Copy link
Member

@soxofaan soxofaan left a comment

Choose a reason for hiding this comment

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

some quick notes

"duration": _ColumnProperties(dtype="str"),
"costs": _ColumnProperties(dtype="float64"),
}

Copy link
Member

Choose a reason for hiding this comment

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

The three comments above should be moved along with _COLUMN_REQUIREMENTS

# Expected columns in the job DB dataframes.
# TODO: make this part of public API when settled?
# TODO: move non official statuses to seperate column (not_started, queued_for_start)
COLUMN_REQUIREMENTS = {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think COLUMN_REQUIREMENTS belongs under the _job_database module.
The requirements describe what the job manager expects as columns, so this should stay close to the job manager module (__init__.py at the moment)

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.

3 participants