-
Notifications
You must be signed in to change notification settings - Fork 1
feat: additional data loggers and enhancements, reducing gateway course limit from 25 to 10 #62
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
Conversation
feat: logs missing "M" course grades oddities feat: add cohort and cohort term breakdown + additional logging statement prints for inference pipeline too fix: capping gateway features at 10 instead of 25 fix: <0.1%% has a double percent sign - changed to "<0.1 percent" for sake of ease (couldn't fix it to just print 0.1%, idk why)
…is triggered currently, the logs were not being updated for the same model run ID but a new inference run. this should fix this (changed mode = "a" (append) to mode = "w" (write). Need to test still.
| except Exception: | ||
| pass | ||
|
|
||
| # --- IMPORTANT PART 2: open in write mode so we overwrite each run --- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are we overwriting here? When we re-run inference, shouldn't that just produce a new run ID subfolder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when we re-run inference if it's the same training model (so if we didn't need to retrain) the logs for some reason aren't getting updated. I still need to test this to ensure it works but we need the logs to be updated to the most recent inference run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this makes me think if under the inference folder, we should have additional sub folders for old & new inference runs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, so then we are only keeping the latest inference run then? I think I remember discussing this. Ideally, we want to have all historical inference run logs as well.
But yeah, for now, I understand we need to prioritize capturing the latest and ensuring that is done correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, we do need to keep old runs
changes & context
Small tweaks/improvements
questions