Skip to content

refactor(setup_schedules): Reduce duplication and improve maintainability. Closes #822#824

Merged
regulartim merged 3 commits intoGreedyBear-Project:developfrom
drona-gyawali:enhance/schedules
Feb 18, 2026
Merged

refactor(setup_schedules): Reduce duplication and improve maintainability. Closes #822#824
regulartim merged 3 commits intoGreedyBear-Project:developfrom
drona-gyawali:enhance/schedules

Conversation

@drona-gyawali
Copy link
Copy Markdown
Contributor

@drona-gyawali drona-gyawali commented Feb 17, 2026 •
edited
Loading

Description

This PR refactors setup_schedules() to remove repeated update_or_create() blocks and centralize cron job definitions.

Related issues

closes: #822

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist

  • I have read and understood the rules about how to Contribute to this project.
  • The pull request is for the branch develop.
  • I have added documentation of the new features.
  • Linter (Ruff) gave 0 errors. If you have correctly installed pre-commit, it does these checks and adjustments on your behalf.
  • I have added tests for the feature/bug I solved. All the tests (new and old ones) gave 0 errors.
  • If changes were made to an existing model/serializer/view, the docs were updated and regenerated (check CONTRIBUTE.md).
  • If the GUI has been modified:
    • I have a provided a screenshot of the result in the PR.
    • I have created new frontend tests for the new component or updated existing ones.

Important Rules

  • If you miss to compile the Checklist properly, your PR won't be reviewed by the maintainers.
  • If your changes decrease the overall tests coverage (you will know after the Codecov CI job is done), you should add the required tests to fix the problem
  • Everytime you make changes to the PR and you think the work is done, you should explicitly ask for a review. After being reviewed and received a "change request", you should explicitly ask for a review again once you have made the requested changes.

@drona-gyawali
Copy link
Copy Markdown
Contributor Author

drona-gyawali commented Feb 17, 2026

HI @regulartim I did not explicitly mention in the issue that I would use a JSON-based configuration file. While refactoring, I noticed a similar pattern with ml_config.json and decided to follow that structure for consistency and maintainability. Is this structure is fine for you?

regulartim
regulartim reviewed Feb 18, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@regulartim regulartim left a comment

Choose a reason for hiding this comment

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

Hey @drona-gyawali !

Is this structure is fine for you?

Yeah, it's fine. But I am not sure what we actually gain by using a json file. Looking at this, I wonder if a python list of dicts in schedules.py would be a better solution. Some drawbacks of the json approach:

  1. introduces file I/O and json parsing, which can fail and should therefore be tested
  2. the file isn't really pure configuration, as it contains hardcoded import paths
  3. it contains magic strings "inteverval" and "calc_train_time"

So as you can see, I am a little skeptical. What I do like however is that the active_schedules are not a separate list anymore.

What do you think about that? What's your opinion on the list of dicts idea?

@drona-gyawali
Copy link
Copy Markdown
Contributor Author

drona-gyawali commented Feb 18, 2026

What do you think about that? What's your opinion on the list of dicts idea?

I just wanted to clarify that the JSON-based file configuration I tried was purely for experimentation to test an idea. I now completely understand your concerns, and I agree that simpler is better.

So I am planing to refactored setup_schedules() to use a single Python function with:

  • One helper _train_cron_minute() for clarity and readability,

  • A single list of dictionaries for all schedule definitions,

  • Everything contained in one file with no external config or extra imports.

This approach preserves all existing functional behavior, so no new tests are required the current tests already cover the tasks themselves. Does this sound good to you?

@regulartim
Copy link
Copy Markdown
Member

regulartim commented Feb 18, 2026

I just wanted to clarify that the JSON-based file configuration I tried was purely for experimentation to test an idea.

That's fine! :)

One helper _train_cron_minute() for clarity and readability,

Don't put too much effort into that. I have an idea how to schedule the training in a better way. Will probably do that soon.

Does this sound good to you?

Yes, sounds good. Feel free to implement.

regulartim
regulartim requested changes Feb 18, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@regulartim regulartim left a comment

Choose a reason for hiding this comment

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

Very nice, I like it! :) I only have two minor suggestions.

Comment thread greedybear/cronjobs/schedules.py Outdated
Comment thread greedybear/cronjobs/schedules.py
@regulartim regulartim merged commit 74f50a4 into GreedyBear-Project:develop Feb 18, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@regulartim regulartim regulartim approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@drona-gyawali @regulartim