Skip to content
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

Add manager host to list of nodes when deploy method is Server #256

Merged
merged 1 commit into from
May 28, 2024

Conversation

myxie
Copy link
Collaborator

@myxie myxie commented May 23, 2024

Issue

This PR addresses #255, in which I was not able to generate and deploy physical graphs because only a single node was being returned to the translator when the ManagerClient was queried for current nodes.

This is only an issue with the Server deploy method; the Browser Direct method is working fine.

Cause

The server and the Browser Direct method use different REST calls:

  • Server: \gen_pg
  • Browser Direct: \gen_pg_spec

This is likely due to their requiring different pieces of information at runtime.

They do get the same list of nodes, albeit in different through different methods. What they do with this list, however, is the difference.

\gen_pg, Server

# 1. get a list of nodes
node_list = mgr_client.nodes()
logger.debug("Calling mapping to nodes: %s", node_list)
# 2. mapping PGTP to resources (node list)
pg_spec = pgtp.to_pg_spec(node_list, ret_str=False)

\gen_pg_spec, Browser Direct

logger.debug("Calling mapping to host: %s", [manager_host] + node_list)
pg_spec = pgtp.to_pg_spec(
[manager_host] + node_list,
tpl_nodes_len=tpl_nodes_len,
ret_str=False,
)

It is clear that for the Browser Direct case (i.e., the one that works) we are adding the manager to the start of any list of nodes, which is what we need when we split the list of nodes into island and node managers in to_pg_spec:

is_list = node_list[0:num_islands]
nm_list = node_list[num_islands:]

Solution

The following is necessary to resolve the issue:

  • Add the manager to the list of nodes for the server (Done)
  • Add some documentation to make explicit what is happening (In-progress)
  • Add more informative error handling when it comes to the node list.

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @myxie - I've reviewed your changes and they look great!

Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.

@coveralls
Copy link

Coverage Status

coverage: 78.622% (-1.1%) from 79.725%
when pulling 89b8d34 on fix_server_genpgt_nodelist
into bf3a8fc on master.

@@ -635,7 +635,7 @@ def gen_pg(
host=mhost, port=mport, url_prefix=mprefix, timeout=30
)
# 1. get a list of nodes
node_list = mgr_client.nodes()
node_list = [f"{mhost}:{mport}"] + mgr_client.nodes()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@awicenec Are you happy with this approach to the fix? I recall you saying in-person you'd made changes to the API instead, but that may have been orthogonal to this issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

These changes are perfect, thanks.

@myxie myxie merged commit 22fb53d into master May 28, 2024
21 checks passed
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.

None yet

3 participants