Refactor Phase 1: Resolve#162
Conversation
houtanb
left a comment
houtanb
left a comment
There was a problem hiding this comment.
Sending PR now since I might not have time again until the weekend. The big blockers here:
- can't run tests locally from a clean install
- it seems like you didn't run orchestration online to ensure it worked as the new resolution file would not have been called by the orchestrator
|
|
||
| resolve-forecasts: | ||
| $(MAKE) -C src/resolve_forecasts || echo "* $@" >> $(MAKE_FAILURE_LOG) | ||
| $(MAKE) -C src/orchestration || echo "* $@" >> $(MAKE_FAILURE_LOG) |
There was a problem hiding this comment.
we spoke about this on Slack, just noting here too to keep the file structure as before. Related from that conversation was to delete all files in src/resolve_forecasts
There was a problem hiding this comment.
Another note: can you modify .github/workflows/makefile.yml to run tests when a PR is created
There was a problem hiding this comment.
Deleted stale files, and also modified the workflows.
Also updated the makefile but not sure how to test that. It's a bit awkward too as my make test activates the .venv but make lint does not. I don't know how you prefer to handle this. I would opt for modifying make lint such that it also activates .venv as if it's activated is not a performance hit at all and adds a layer of safety.
Also, tbh, not 100% sure whether make setup-python-env would run smoothly -- I am guessing there would be a reason you used pip install before, so that makes me a bit hesitant.
There was a problem hiding this comment.
Feel free to experiment with what works best here.
| # Attach warnings for orchestration to send via Slack | ||
| if warnings: | ||
| existing = df.attrs.get("_resolve_warnings", []) | ||
| df.attrs["_resolve_warnings"] = existing + warnings |
There was a problem hiding this comment.
Instead of _resolve_warnings and warnings call these _warnings_for_slack to make the different treatment apparent (different as opposed to other warnings/info/error messages that are logged during processing in _resolve() functions).
There was a problem hiding this comment.
These functions should perhaps return a (df, slack_warnings) tuple as the attrs could unintentionally be erased on modification of df.
There was a problem hiding this comment.
done as requested, though let's circle back eventually, while i prefer the tuple return, it's still a tad wonky looking, I'd prefer to design a dedicated warning/error mechanism that's a bit cleaner than this; perhaps part of a logging module/setup
There was a problem hiding this comment.
I agree that would be the best solution. Noting that I am concerned a bit with the risk of losing these slack messages. Can you think of a test we can implement to ensure slack warnings are not overwritten in the process.
nikbpetrov
force-pushed
the
resolve
branch
2 times, most recently
from
18b687d to
98b66c4
Compare
|
merged in 7f6c3cd |
2 participants
Current code runtime: 81mins (fastest job finishes in 41mins)
Refactored code runtime: 78mins (fastest job finishes in 44mins)
Outputs of both jobs (resolution sets and processed forecast sets) are identical, and both match the production data.
IMPORTANT: Before merging rename the job (
src/orchestration/Makefile) tofunc-resolve-forecasts.