Replace partition by rpartition for parsing app_id #517
Conversation
|
Hi @ljjsalt! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
facebook-github-bot
added
the
CLA Signed
There was a problem hiding this comment.
Hi, thanks for contributing!
The one thing I'm a bit worried about is in the cases where there's a dash in the ID since that's valid in all of our schedulers.
Since I believe the ray host address always contains the port (and IDs can't contain :) it might be best for us to do a combination where we look look for the first :\d+- and split on that instead.
Can pull out the partition logic into a helper method and would be nice to add some tests as well
foo-foo:1234-bar-bar
|
Looks like that's the case--the ray integration tests are failing because of the right dashes |
Codecov Report
@@ Coverage Diff @@
## main #517 +/- ##
==========================================
- Coverage 94.78% 90.27% -4.52%
==========================================
Files 65 65
Lines 3932 3938 +6
==========================================
- Hits 3727 3555 -172
- Misses 205 383 +178
Continue to review full report at Codecov.
|
Yes, this makes sense. So can we make sure that there is always at least one ":" in the host address and should we consider the situation that there are more than one ":"? |
| parsed_addr, parsed_app_id = self._scheduler._parse_app_id( | ||
| f"{addr}:{port}-{app_id}" | ||
| ) | ||
| self.assertEqual(parsed_addr, addr) |
There was a problem hiding this comment.
I think this is dropping the port -- port should still be included in the address
Would also be good to add an explicit test where there's no port specified and it should fall back to the old behavior for safety reasons
There was a problem hiding this comment.
My bad, I didn't see the instruction on how to run tests in the document.
| addrs = ["0.0.0.0", "www.testexample.com", "addr-of-cluster"] | ||
| ports = ["65535", ] | ||
| app_ids = ["a-p-p_id", "app-i-d", "app_id", "app:id"] | ||
| for addr, port, app_id in product(addrs, ports, app_ids): |
There was a problem hiding this comment.
product is clever but I think it's a bit easy to miss things since it's a bit hard to follow
no strong preference but might be better to just be very explicit and call _parse_app_id for each case even if it's a few more lines of code
There was a problem hiding this comment.
OK, I have changed the test examples to explicit form.
Include port number in parsed cluster address
|
@d4l3k has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
I made some fixups on this at #523 @ljjsalt the commit should still link to your account if you add your @ibm.com email to your GitHub account |
|
ok no problem |
|
landed via 5d1ad55 |
To parse a address like
"addr-of-cluster-app_id", the old code will fail since it takes"addr"as the address and"of-cluster-app_id"as the app id.After replace
partitionbyrpartition, it should work as intended.Test plan:
Follow the original test.