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

Linter changes, fixes scheduled index job #348

Merged
merged 4 commits into from
Jan 12, 2024
Merged

Conversation

MattWellie
Copy link
Collaborator

@MattWellie MattWellie commented Jan 8, 2024

Fixes

  • The report hunter process (collect all previous AIP reports into an HTML index page) doesn't work, and hasn't worked in ages
  • It requires a docker image with specific libraries installed, and values in config that weren't previously satisfied in the chrono-triggered runs

Proposed Changes

  • Runs the whole process in the AIP image (built on top of the driver image as a base, so all dependencies satisfied)
  • No expectation that a cohorts section of the config file exists (no need for this check)
  • All functional changes here are contained in .github/workflows/index_page_builder.yaml (only run twice a week, not 3 times. Run in the AIP docker image) and report_hunter.py (remove part of the config check), everything else is linting

Additional

  • Following suggestion here, alters the pre-commit hooks, and generates a ton of changes due to the addition of isort

@MattWellie MattWellie requested a review from illusional January 10, 2024 22:58
Copy link
Contributor

@illusional illusional left a comment

Choose a reason for hiding this comment

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

This is your repo, so I'll let you own the title, but this to me is a Linting PR + deploy docker update and I think the PR title should reflect that to make it easier in the future to make it easier to ignore the commit in blames (and be more descriptive in git logs).

.github/workflows/index_page_builder.yaml Show resolved Hide resolved
@MattWellie
Copy link
Collaborator Author

This to me is a Linting PR + deploy docker update and I think the PR title should reflect that

Yeah this was rough timing - it was an isolated workflow fix, then you mentioned isort on Slack. I didn't think it would change quite so many files... Title is now pretty misleading, so i'll sort that out

@MattWellie MattWellie changed the title Fix Report-Hunter script Linter changes, fixes scheduled index job Jan 12, 2024
@MattWellie MattWellie merged commit 0dac8a8 into main Jan 12, 2024
4 checks passed
@MattWellie MattWellie deleted the fix-report-hunter branch January 12, 2024 01:02
kdahlo pushed a commit to jeremiahwander/automated-interpretation-pipeline that referenced this pull request Jan 17, 2024
* introduces isort, and applies to all files
* fixes report hunter cron-triggered workflow, reduces frequency
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