Skip to content

Fix binding when user sets HTEx worker ports manually #3838

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

WardLT
Copy link
Contributor

@WardLT WardLT commented Apr 7, 2025

Description

Fixes the creation of the TCP url when the ports used by HTEx workers are fixed.

The tcp_url helper function from addresses.py was only appending the port number when the address was not "*".

Changed Behaviour

HTEx works with the ports fixed. Not sure when it was broken

I added a unit test which shows that the current code fails when launching an HTEx interchange with user-set ports.

Fixes

None (yet? should I make one?)

Type of change

  • Bug fix

@WardLT WardLT force-pushed the fix-zmq-fixed-bindings branch 2 times, most recently from 56d75ad to 450f44b Compare April 7, 2025 01:02
@WardLT WardLT force-pushed the fix-zmq-fixed-bindings branch from 450f44b to 41dd76c Compare April 7, 2025 01:04
@WardLT WardLT marked this pull request as ready for review April 7, 2025 01:57
@WardLT
Copy link
Contributor Author

WardLT commented Apr 7, 2025

Might I suggest @yadudoc for the review? I altered a function he wrote a few months ago

@benclifford benclifford requested a review from yadudoc April 7, 2025 07:49
@benclifford
Copy link
Collaborator

you might add a case into parsl/tests/unit/test_address.py which more directly tests the tcp_url function

Copy link
Collaborator

@khk-globus khk-globus left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test. We're working through a build error at the moment, but I'll merge this once that gets sorted. Thanks!

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.

3 participants