Fix policy update test #365
Conversation
meta-cla
bot
added
the
CLA Signed
|
Can we close #143 once this lands too :)? |
| ) | ||
|
|
||
| # Cleanup DCP directory | ||
| path = Path(TEST_DCP_DIR) |
There was a problem hiding this comment.
You can use TemporaryDirectory which handles this automatically
There was a problem hiding this comment.
I have to change this because NFS does not work for tmp directory
| # We only care about the final output | ||
| params.output_kind = RequestOutputKind.FINAL_ONLY | ||
| return params | ||
|
|
There was a problem hiding this comment.
Can you explain why this has to be in policy.py? Is monarch failing to pickup/serialize the function if it's not in here?
If it has to be here, may be prepend all the functions with _.
There was a problem hiding this comment.
Yes, monarch pickled the function with the module path integration_tests but this cannot be resolved in the remote node. Seems that we need to define it in the main modules.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #365 +/- ##
=======================================
Coverage ? 63.24%
=======================================
Files ? 78
Lines ? 7725
Branches ? 0
=======================================
Hits ? 4886
Misses ? 2839
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
While the test is amazing, having it in the policy.py is not really an option IMO.
From a UX / library perspective, it makes things way more difficult to understand. The argument could be made that directing users to docs alleviates this pain, but 1) our docs are incredibly incomplete and 2) we know from OSS users experience with titan/ tune that most people prefer to just go to the code.
Oh I completely missed that part. Is it possible to create an inherited Policy that has all of the test functionality and is kept in integration_tests? I'm not sure I'm clear what the monarch pickling issue is |
|
@allenwang28 What are our options here? Surely someone else has tried to run a multi node test via Monarch and not have to clutter up an Actor? |
|
Wild thought (lmk if this errors): For tests specific methods, can we just append them to the class definition in the test? It's the same idea as inheriting, but let's you surgically change the nested PolicyWorker Within test: from foo import Bar
def util(self):
...
Bar._special_test_util = util
proxy: Bar = Bar()
I'd like to see the trace too since at least 2 others have encountered this when using Monarch/Services |
6 participants
Specifically,
policy.pymodule because@endpointalmost only work with lambda functions.