Skip to content

[WIP] Add Provisioner #2159

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

Closed
wants to merge 8 commits into from
Closed

Conversation

gcatron
Copy link
Contributor

@gcatron gcatron commented Dec 11, 2018

Description: This is the interface for the Runtime Provisioner, implementation to follow.
Testing: not yet
Documentation: N/A
Related to #2045

/// DeviceManager and uses the returned networkID to populate \p runDAG.
/// Returns a ResultCode indicating if the operation was a success.
ResultCode provision(dependencyDAG &networks, executionDAG &runDAG,
std::unordered_map<int, DeviceManager> &devices);
Copy link
Contributor

Choose a reason for hiding this comment

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

After this stage, does runDAG keep all the sub-modues and DAG used for execution? I"dependencyDAG &networks" is not needed anymore ?

In addition, I think the class HostManager need to be updated based on current discussion :)

Copy link
Contributor Author

@gcatron gcatron Dec 11, 2018

Choose a reason for hiding this comment

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

no runDAG is an executionDAG so it contains the dependencies and networkIDs. After this stage the sub-modules should all be loaded onto the devices so the host does not need to keep them around.

I agree! The HM is quite out of date, I plan to update it once the other components have settled.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest defining a typealias like DeviceManagersMap instead of using the explicit unordered_map here.

FAILED,
CANCELLED
}; // This will be defined in one common runtime place.
class executionDAG;
Copy link
Contributor

Choose a reason for hiding this comment

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

vertical whitespace here?

class executionDAG;
class dependencyDAG;
class DeviceManager;
/// The Provisioner is responsible for assigning networks to an actual device.
Copy link
Contributor

Choose a reason for hiding this comment

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

And here?

Copy link
Contributor

@bertmaher bertmaher left a comment

Choose a reason for hiding this comment

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

I've a trivial request: some vertical whitespace O:-)

FAILED,
CANCELLED
}; // This will be defined in one common runtime place.
class executionDAG;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you move the forward declarations right after the line namespace glow {? It looks better that way and we usually put forward declarations at the very top.

/// devices. The Provisioner calls the addNetwork method for each
/// DeviceManager and uses the returned networkID to populate \p runDAG.
/// Returns a ResultCode indicating if the operation was a success.
ResultCode provision(dependencyDAG &networks, executionDAG &runDAG,
Copy link
Contributor

Choose a reason for hiding this comment

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

dependencyDAG and executionDAG are type names. They should start with uppercase.

/// DeviceManager and uses the returned networkID to populate \p runDAG.
/// Returns a ResultCode indicating if the operation was a success.
ResultCode provision(dependencyDAG &networks, executionDAG &runDAG,
std::unordered_map<int, DeviceManager> &devices);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest defining a typealias like DeviceManagersMap instead of using the explicit unordered_map here.

// On success add the deviceID to the executionDAG
// If a module fails to provision try on a different device if available.

};
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing line feed.

#include <unordered_map>

namespace glow {
enum ResultCode {
Copy link
Contributor

Choose a reason for hiding this comment

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

Doxygen comment for ResultCode?

@gcatron
Copy link
Contributor Author

gcatron commented Jan 8, 2019

Wasn't paying attention and this got a little unwieldy. I'm keeping it open for now, but I intend to create a new cleaner PR when the provisioner is ready.

@stale
Copy link

stale bot commented Jan 13, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@gcatron
Copy link
Contributor Author

gcatron commented Jan 17, 2019

Closing as a new PR #2276 has been made.

@gcatron gcatron closed this Jan 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants