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

Complete several of the utility documents for Jupyter #32

Merged
merged 12 commits into from Apr 29, 2024

Conversation

cmickeyb
Copy link
Contributor

@cmickeyb cmickeyb commented Mar 29, 2024

At the request of @prakashngit , combining #33 .

Update tests for groups

Replace invocation of the script for building a local groups and services database with one that uses the public interfaces for the services and groups databases. Move away from "host specific" configuration to a configuration based on the more general site configuration file (i.e. site.toml).

By default, testing will still occur with locally hosted services. Importing a site configuration file from another service provider (which just involves copying the site configuration file to ${PDO_HOME}/etc/sites/${SERVICE_HOST}.toml) enables use of PDO services from any location.

Incorporate new widgets

This adds support for several utility notebooks that can be used for managing a client configuration. The notebooks themselves are included along with a collection of widgets that can be used by contract-specific notebooks. The widgets generally include ones to create lists of available resources (e.g. keys, services, groups), select amongst a list of available resources, and create or update resources.

  • key-manager: manage keys available to the client
  • service-manager: manage the database of services that can be used for contracts
  • service-groups-manager: manage the database of service groups used for contracts

Copy link
Contributor

@prakashngit prakashngit left a comment

Choose a reason for hiding this comment

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

lots of interesting features. would have to think more about how these get used when we build a demo such as the one that are doing now.

# common sections:
#
# * Configure the Contract Object
# * Initialize the Interpreter and the PDO Environment
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. not sure what is meant by configure the contract object (specifically, how is this related to initialize contract object context)
  2. initialize the interpret -> you mean initialize the client?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

please suggest wording changes.

Copy link
Member

Choose a reason for hiding this comment

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

+1 to Prakash's comment. Do we have documentation on contract configuration? If so, one possible edit here would be to link "Configure the Contract Object" to this documentation. Also, which interpreter is being initialized? Is this talking about WAMR? Here's a suggestion, if that's what you mean here.

Suggested change
# * Initialize the Interpreter and the PDO Environment
# * Initialize the PDO Runtime Environment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Section rewritten. However, this really is about the Jupyter interpreter and the PDO Environment.

# execution.
#
# The next section in the notebook configures information about the contract object associated with
# the notebook. In some cases, the variables will set by the notebook using the Papermill
Copy link
Contributor

Choose a reason for hiding this comment

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

"The next section in the notebook " and the rest of the of the documentation in this section. sounds like what "this specific notebook does" whereas I guess you are referring to notebook that is used to interact with contracts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

suggest wording changes.


children = map(lambda stype : ServiceListWidget(state, bindings, stype), service_labels.keys())
service_list = ipywidgets.Tab(children=list(children), titles=list(service_labels.values()))
ip_display.display(ipywidgets.VBox([service_list, ServiceUploadWidget(state, bindings)]))
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like you assume that by default there are services deployed on localhost? What if there all services are located on remote only? I see that there is an option to upload the site.toml file, but the option is presented only after a default setting on localhost is executed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where do you see localhost? there are no assumptions. yes, test_jupyter sets up a localhost site (that's where the services are located). but you can load any site file. i do this often for other "permanent" services.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, when I ran the notebook, my observation was the I get the option to upload the site.toml only after executing the cell; but while executing the cell that it also first showed the existence of a default localhost based group. My question was what happens if such a default localhost group does not exist. I have no way to test it now, until I deploy a more elaborate set up.

Copy link
Contributor Author

@cmickeyb cmickeyb Apr 8, 2024

Choose a reason for hiding this comment

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

yes. we had already loaded that site.toml for the localhost test files. to be clear... if you run start_jupyter.sh you will not have any information pre-loaded. if you run the tests we intentionally load the local site.toml (which for the test configuration comes from the services running on localhost). so YES, the test environment pre-configures groups for localhost as expected. but that is not the case for a "running system".

# Additional management of service group information is provided by the [Service Groups Manager
# Notebook](service_groups_manager.ipynb)
# %%
from pdo.contracts.groups import ServiceGroupListWidget
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that there is special emphasis on the service db and serivce groups, would be better if "contract context specification" includes an option for the contract author to specify which service group to use to deploy the contract. The Issuer notebook only specifies the service host where the contract objects will be created; what seems more meaningful is to rather specify the service groups (even if it is default) where the contract will be deployed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that change is coming in the contracts. i have a PR ready to go that updates all of those in the exchange cf. to be clear... you will not specify a "host" but rather will specify a group.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks, looking forward to that PR.

# ## Create a New Storage Service Group
#
# %%
ip_display.display(StorageServiceGroupCreateWidget(state, bindings))
Copy link
Contributor

Choose a reason for hiding this comment

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

(perhaps not a comment for this specific notebook:):

How do we ensure that the notebook user understands that the storage service group needs to be "compatible" with the eservice group when provisioning a contract? might be good to document this aspect somwhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but it doesn't need to be compatible. the client will load the contract to the eservice. the storage services are exclusively for your replication protocol, NOT for use in eservices. that being said... a really smart/performant policy would connect the two. but there is no requirement that they must be the same.

Copy link
Contributor

Choose a reason for hiding this comment

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

don't we need to assume that the storage group for sure includes sservices associated with the provisioned eservices? this would be the default assumption. We need to add more sservices to the storage group if the replication policy calls for additional redundancy.

Copy link
Contributor Author

@cmickeyb cmickeyb Apr 8, 2024

Choose a reason for hiding this comment

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

we do not. reminder that the storage services are strictly for replication & availability. state is always provisioned (from the client) to an enclave service prior to execution. these are two completely distinct concepts.

# does not require service verification, it is often useful to check on the
# status of services.
# %%
def verify_service(service_type, urls, output) :
Copy link
Contributor

Choose a reason for hiding this comment

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

"verification" term is a bit overloaded/confusing. I suppose within the esrvicedb, we verify each service against ledger, and record "ledger verified time in the db". whereas looks like in here "verify" means a "healthcheck" of the service itself that the client gets (via the /info url). how are these two verifications related?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in this case, not really. the verify function does exactly that... it makes sure that the service exists and that the identity has not changed. this is old code.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok thanks, so if I understand correctly, verify here does not verify against the ledger, just a healthcheck of the service.
thinking aloud, for a contract user, sepcifically one who just wants to participate in an already deployed PDO-based application (such as new token owner), what would be useful is for the user to start with the application context file, and invoke a verify API on the context file, and behind the scenes, the validity of the services specified by the whole context file gets verified (including ledger checks for attestation/registry info, service heartbeats such as what you are doing here etc.). We should not be expecting the user (I mean the new token owner) to maintain (or at least understand) any other config file other than the context. PDO specific configs such as servicedbs, groupdbs must be constructed transparently based on the info found in context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the framework for verification can be extended in the future (this is a conversation suggested by @g2flyer ). however, FOR THE MOMENT, this introduces no new functionality; it simply exposes the ability to perform an operation that has been part of PDO for "months". i would suggest we start an issue in the PDO repository to track this discussion.

@cmickeyb
Copy link
Contributor Author

cmickeyb commented Apr 7, 2024

lots of interesting features. would have to think more about how these get used when we build a demo such as the one that are doing now.

not intended for a demo. more intended for managing information for a pilot.

@cmickeyb cmickeyb marked this pull request as ready for review April 16, 2024 16:51
@cmickeyb cmickeyb requested a review from g2flyer April 17, 2024 14:46
@g2flyer
Copy link

g2flyer commented Apr 17, 2024

FYI: make -C docker pdo_build_images does not work anymore. However, the issue is in private-data-objects. I thought initially i might be the culprit in review of PDO PR #477 but now noticed that even the old version would have failed. For the all target to work the sgx special build targets would seem to have to be called build_service_sgx (and not as originally build_sgx_serviceor as changed due to my consistency changesgx_build_service`). Let me see if this quick-fix really works and i will create a corresponding PDO PR ...

@cmickeyb
Copy link
Contributor Author

FYI: make -C docker pdo_build_images does not work anymore. However, the issue is in private-data-objects. I thought initially i might be the culprit in review of PDO PR #477 but now noticed that even the old version would have failed. For the all target to work the sgx special build targets would seem to have to be called build_service_sgx (and not as originally build_sgx_serviceor as changed due to my consistency changesgx_build_service`). Let me see if this quick-fix really works and i will create a corresponding PDO PR ...

i've noticed that we are now superceding "all" as the default target. and in part its because of where we included make.loc. explicitly calling make with a target should fix this. I can add a small commit or we can just handle it separately.

@g2flyer
Copy link

g2flyer commented Apr 17, 2024

i've noticed that we are now superceding "all" as the default target. and in part its because of where we included make.loc. explicitly calling make with a target should fix this. I can add a small commit or we can just handle it separately.

this PR should fix the issue without change here (of course whether target all is the right one here is a different question but all shouldn't fail ...)

Copy link

@g2flyer g2flyer left a comment

Choose a reason for hiding this comment

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

Changes look good -- definitely like the disappearance of some skip's (although still many left ;-) -- and after applying locally the pdo-build-target-fix and switching again test-machines as it obscurely fails on machines which do not support AVX -- still think my previous suggestion for a simple test would be useful .. - also builds from standalone repo. However, running it, i run into some issues

  • the inference token issuer notebook creation displays rather than renders [Newly created token issuer](../../instances/inference/user1 tokens/token-issuer_DQLVoj.ipynb), i.e., it is not clickably anymore as in earlier versions
  • pdo env initialization fails for issuer notebook due to No such file or directory: '/project/pdo/run/opt/pdo/bin/pdo-create-service-groups.psh', the rest of that notebook though still worked
  • the "Notebook to Invoke Token Operations" still seemed to have a number of past open comments (e.g., to show in text path to zebra image file on docker) and being too lazy to search that path, i gave up for now and wait until these prior comments are resolved

@cmickeyb
Copy link
Contributor Author

Changes look good -- definitely like the disappearance of some skip's (although still many left ;-) -- and after applying locally the pdo-build-target-fix and switching again test-machines as it obscurely fails on machines which do not support AVX -- still think my previous suggestion for a simple test would be useful .. - also builds from standalone repo. However, running it, i run into some issues

  • the inference token issuer notebook creation displays rather than renders [Newly created token issuer](../../instances/inference/user1 tokens/token-issuer_DQLVoj.ipynb), i.e., it is not clickably anymore as in earlier versions
  • pdo env initialization fails for issuer notebook due to No such file or directory: '/project/pdo/run/opt/pdo/bin/pdo-create-service-groups.psh', the rest of that notebook though still worked
  • the "Notebook to Invoke Token Operations" still seemed to have a number of past open comments (e.g., to show in text path to zebra image file on docker) and being too lazy to search that path, i gave up for now and wait until these prior comments are resolved

Presumably these issue are all in the inference notebooks. I need to go through & update them (they did not exist when this PR was submitted). Good catch!

@cmickeyb
Copy link
Contributor Author

Changes look good -- definitely like the disappearance of some skip's (although still many left ;-)

The only way to get rid of the %%skips is to move entirely to interactive operations. I think this PR introduces many of the widgets necessary to do that, but its still low on my priority list.

cmickeyb and others added 9 commits April 22, 2024 09:52
Add service groups and support for SGX-enabled docker tests.

Signed-off-by: Mic Bowman <cmickeyb@gmail.com>
Replace invocation of the script for building a local groups and
services database with one that uses the public interfaces for the
services and groups databases. Move away from "host specific"
configuration to a configuration based on the more general site
configuration file (i.e. site.toml).

By default, testing will still occur with locally hosted services.
Importing a site configuration file from another service provider
(which just involves copying the site configuration file to
${PDO_HOME}/etc/sites/${SERVICE_HOST}.toml) enables use of PDO
services from any location.

Signed-off-by: Mic Bowman <mic.bowman@intel.com>
Add a check to make sure that jupytext has been installed.
Currently this is a fatal error.

Signed-off-by: Mic Bowman <mic.bowman@intel.com>
Creation and management of the service groups database is
now provided directly with pdo-service-groups command. There
is no longer a need for the (very limited) old script.

Signed-off-by: Mic Bowman <mic.bowman@intel.com>
Substantial shift towards widgets as classes instead
of widgets as functions. This allows for key handling
and widget reset functions to be bundled more easily.

Key manager is now more or less complete.

Signed-off-by: Mic Bowman <mic.bowman@intel.com>
The services manager page (and supporting functions and widgets)
allows for viewing, selecting and importing lists of enclave,
provisioning and storage services.

Signed-off-by: Mic Bowman <mic.bowman@intel.com>
This commit depends on the PDO update for persistent service
groups database.

Add a jupyter notebook for creating, managing and using service
groups. This update includes widgets to simplify selection and
use of service groups.

Also adds a jupyter notebook for demonstrating different widgets
that are not part of the management pages.

Signed-off-by: Mic Bowman <mic.bowman@intel.com>
Replace the "documentation only" form of the getting started notebook
with one that provides the functionality for actually checking and
fixing the PDO configuration. The new form walks the user through key
creation, service endpoint import, and service group creation. This
should ensure that there is a fully configured environment for testing
services.

Signed-off-by: Mic Bowman <mic.bowman@intel.com>
General clean up of the install and jupyter documentation that is
generally intended to be read outside of the notebook interface.

Signed-off-by: Mic Bowman <mic.bowman@intel.com>
Include the new modules in the exchange manifest
Include ipywidgets in the package dependencies

Signed-off-by: Mic Bowman <cmickeyb@gmail.com>
Copy link
Member

@marcelamelara marcelamelara 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 reviewed the introductory text so far. Will try to review the rest of this doc later.

Comment on lines 26 to 27
# contract from one of the contract specific templates.The templates provided generally share five
# common sections:
Copy link
Member

Choose a reason for hiding this comment

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

It looks like these 5 sections generally map to common contract APIs. If so, I think it'd be useful to just say that explicitly since users should ideally still think of contracts as programs, no?

Suggested change
# contract from one of the contract specific templates.The templates provided generally share five
# common sections:
# contract from one of the contract specific templates. The templates provided include five
# sections mapping to the common contract APIs:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they do not. they are about the user interface that we happen to use.

# common sections:
#
# * Configure the Contract Object
# * Initialize the Interpreter and the PDO Environment
Copy link
Member

Choose a reason for hiding this comment

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

+1 to Prakash's comment. Do we have documentation on contract configuration? If so, one possible edit here would be to link "Configure the Contract Object" to this documentation. Also, which interpreter is being initialized? Is this talking about WAMR? Here's a suggestion, if that's what you mean here.

Suggested change
# * Initialize the Interpreter and the PDO Environment
# * Initialize the PDO Runtime Environment

# * Configure the Contract Object
# * Initialize the Interpreter and the PDO Environment
# * Initialize the Contract Object Context
# * Operate on the Contract
Copy link
Member

Choose a reason for hiding this comment

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

This sounds a bit like the intent is to be able to manipulate the contract (i.e., re-configure the contract), rather than interact with its functions. I tried to find a term that matches the mental model of interacting with a contract more closely.

Suggested change
# * Operate on the Contract
# * Invoke Contract Functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes and no. The point is not to expose the methods. The point is to provide "user appropriate" functions that operate on one or in some cases several contracts to do something.

Copy link
Member

Choose a reason for hiding this comment

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

ah I see, so this is really a level of abstraction above individual contract method invocations? The other concern I have with "operate on a contract" is that, to me, it sounds a lot like configuration, so I would suggest making the distinction between the config step and the operate step a bit more explicit.

# * Initialize the Interpreter and the PDO Environment
# * Initialize the Contract Object Context
# * Operate on the Contract
# * View Contract Metadata
Copy link
Member

Choose a reason for hiding this comment

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

As above, I would add a link to Metadata documentation here, if that exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Future work.

# * Operate on the Contract
# * View Contract Metadata
#
# To initialize the interpreter, the notebook loads the Juptyer helper module. This module imports
Copy link
Member

Choose a reason for hiding this comment

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

I think we might want to abstract away the interpreter pieces since contract users will possibly view the PDO environment more simply as a "runtime".

Suggested change
# To initialize the interpreter, the notebook loads the Juptyer helper module. This module imports
# To initialize the PDO runtime, the notebook loads the Juptyer helper module. This module imports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no. it really is the interpreter.

Comment on lines 39 to 43
# In addition, the interpreter initialization configures an IPython extension that makes it easier
# to provide code for multiple types of operations that can be performed on the contract
# object. Specifically, the extension defines a magic keyword, `skip`, that takes a value or
# expression that, if it evaluates to True, causes the code section to be skipped during notebook
# execution.
Copy link
Member

Choose a reason for hiding this comment

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

Is this saying that the notebook is capable of taking arbitrary user-defined code to interact with the contract?

Separately, I wonder if we should eventually (in another PR) create separate documentation for the inner workings of the PDO environment that underlies all of this to simplify this getting started doc further.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to the first... um... yeah. jupyter notebooks are interpreter instances. they are interpreted python and you can do pretty much anything you want in the notebook cells. the security model for jupyter is basically local access only with password or token access. its more or less like running any other interactive shell.

to the second... yes. add an issue?

# execution.
#
# The next section in the notebook configures information about the contract object associated with
# the notebook. In some cases, the variables will set by the notebook using the Papermill
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# the notebook. In some cases, the variables will set by the notebook using the Papermill
# the notebook. In some cases, the variables will be set by the notebook using the Papermill

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this section has been rewritten.

# extensions. In some cases, the variables may be customized for the specific behavior desired.
#
# The following section initializes the PDO environment from the PDO configuration files. There may
# be opportunities for customization, but so long as the PDO configuration is complete changes to
Copy link
Member

Choose a reason for hiding this comment

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

nit

Suggested change
# be opportunities for customization, but so long as the PDO configuration is complete changes to
# be opportunities for customization, but so long as the PDO configuration is complete, changes to

#
# The final common section initializes the contract context. Where the PDO initialization handles
# configuration of the PDO modules, this section handles configuration of the specific contract
# object and its relationship to other contract objects.
Copy link
Member

Choose a reason for hiding this comment

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

A couple of thoughts. Are we missing a description of one of the five sections? I only see four. Also, they are out of order compared to the bulleted list above, so whichever order makes the most sense is fine, but the ordering of that list and these paragraphs should be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. fixed.

Mostly focused on updating the content in the section on "How to Use
Contracts with Jupyter".

Signed-off-by: Mic Bowman <mic.bowman@intel.com>
# * View Contract Metadata
#
# *Configure the Contract Object*: Each notebook either loads an existing contract object from a
# file (see the PDO documentation on contract collections) or creates a new contract object based on
Copy link
Contributor

Choose a reason for hiding this comment

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

I think "Configure the Contract Object" is perhaps misleading: We are "configuring the notebook" (using the factory notebook as a base) as required to interact with the contract

# usually sufficient.
#
# *Operate on the Contract*: The contract object may be used once it has been created and
# initialized. In general, cells in this section of the notebook are turned off by default; that is,
Copy link
Contributor

Choose a reason for hiding this comment

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

"Initialize the Contract Object Context" -> Perhaps change this to "Initialize the application context" by specifying the contracts invovled in the application, and also the inter-dependency among the contracts used by the applicaiton.

# *Operate on the Contract*: The contract object may be used once it has been created and
# initialized. In general, cells in this section of the notebook are turned off by default; that is,
# `%%skip True` is added at the top of the cell. To perform an operation, change the top line to
# `%%skip False` and evaluate the cell.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above "Operate on the contract" -> Perhaps change this to "operate on the app"

Copy link
Contributor Author

@cmickeyb cmickeyb Apr 24, 2024

Choose a reason for hiding this comment

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

Same comment as above "Operate on the contract" -> Perhaps change this to "operate on the app"

btw... i really hope you've spent this much time on the other new notebooks out there. and the new tests. and the new code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

while i don't disagree... the metaphor for a notebook (app? contract?) runs through much of our documentation. can i suggest that rather than microanalyze it in one document, that we step back and address the bigger question of terminology (and consistency with it) in an issue? i'm tempted to completely remove this section & leave this document focused on operational configuration.

Copy link
Member

@marcelamelara marcelamelara 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 the edits @cmickeyb . I mostly have comments about making existing documentation easier to find at this point.

# * View Contract Metadata
#
# *Configure the Contract Object*: Each notebook either loads an existing contract object from a
# file (see the PDO documentation on contract collections) or creates a new contract object based on
Copy link
Member

Choose a reason for hiding this comment

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

Nit: can we add a link to the referenced documentation? It'll be much easier for the reader to find the right information if we provide the link directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not really. the only way to reference the PDO documentation would be through the git repository.

# execution.
#
# *Initialize the Contract Object Context*: The next section in the notebook creates a PDO context
# for the contract object (see the PDO documentation for more information on contexts). The context
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, if the documentation already exists, we should add a link here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed... and if you have a suggestion for how to do that I would happily incorporate. the closest that we have is to put in a ref to the pdo git hub repo. not my favorite solution.

# ## Import Site Information
#
# PDO clients require information about the services that will be used for contracts. Typically, a
# service provider will create a site description file (often called `site.toml`) that can be used
Copy link
Member

Choose a reason for hiding this comment

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

What are the assumptions about where PDO clients can obtain site.toml files to import?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we have none per se. for the moment, my preference is to keep that "out of band".

Copy link
Member

Choose a reason for hiding this comment

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

Alright, that's fine. I always like to make assumptions like these explicit (if they aren't already somewhere) to avoid any confusion.

docs/notebooks/documents/getting_started.py Outdated Show resolved Hide resolved
Removed the explanatory text from the getting started
notebook. This makes the notebook more operational; it
becomes a single point of performing the most common
operations for configuration.

Cleaned up some text from a couple books (dropped WIP).

Put the key generation functions into a single tabbed
interface.

Signed-off-by: Mic Bowman <mic.bowman@intel.com>
Copy link
Contributor

@prakashngit prakashngit left a comment

Choose a reason for hiding this comment

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

Thanks, tests work as long as pdo images have been generated. if you are using the pdo submodule referenced to by this PR, please note that pdo images need to be generated directly from within the pdo folder, rather than using the make build_pdo_images. This fix for this pdo-related (and not specific to this PR) appears in future pdo commits. This will become a non-issue once we update the pdo submodule for this contracts in later PR.

Copy link
Member

@marcelamelara marcelamelara left a comment

Choose a reason for hiding this comment

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

Thanks! Just one small edit to the text at the top.

Comment on lines +18 to +19
# This notebook provides an aggregate interface for preparing to work with PDO contracts through the
# necessary steps to set up a functional configuration. It assumes that the reader is familiar with
Copy link
Member

Choose a reason for hiding this comment

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

The first sentence is a little convoluted.

Suggested change
# This notebook provides an aggregate interface for preparing to work with PDO contracts through the
# necessary steps to set up a functional configuration. It assumes that the reader is familiar with
# This notebook provides an aggregate interface for preparing to work with PDO contracts, and describes the
# necessary steps to set up a functional configuration. It assumes that the reader is familiar with

# ## Import Site Information
#
# PDO clients require information about the services that will be used for contracts. Typically, a
# service provider will create a site description file (often called `site.toml`) that can be used
Copy link
Member

Choose a reason for hiding this comment

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

Alright, that's fine. I always like to make assumptions like these explicit (if they aren't already somewhere) to avoid any confusion.

@g2flyer g2flyer self-requested a review April 29, 2024 19:06
Copy link

@g2flyer g2flyer left a comment

Choose a reason for hiding this comment

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

Approve as make -C test works and inference contract issues i pointed out in #32 (review) are deferred to PR #37

@prakashngit prakashngit merged commit 02c0718 into hyperledger-labs:main Apr 29, 2024
2 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

4 participants