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

Update documentation #411

Merged
merged 32 commits into from
Dec 13, 2023
Merged

Conversation

al-rigazzi
Copy link
Collaborator

@al-rigazzi al-rigazzi commented Nov 8, 2023

This PR adds a section, named ML Features, to the SmartSim documentation. The section currently has subsections explaining how to upload PyTorch, TensorFlow, and ONNX models and functions to the DB.
The Online Analysis tutorial notebook was also enriched with TorchScript post-processing functions which show how the TorchScript mechanism works.
Other minor typos and documentation inconsistencies were fixed as part of this PR.

@al-rigazzi al-rigazzi self-assigned this Nov 8, 2023
@al-rigazzi al-rigazzi added the area: docs Issues related to documentation label Nov 8, 2023
Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Merging #411 (fa20778) into develop (1b92adf) will not change coverage.
Report is 1 commits behind head on develop.
The diff coverage is n/a.

❗ Current head fa20778 differs from pull request most recent head a08f4c5. Consider uploading reports for the commit a08f4c5 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #411   +/-   ##
========================================
  Coverage    90.24%   90.24%           
========================================
  Files           60       60           
  Lines         3864     3864           
========================================
  Hits          3487     3487           
  Misses         377      377           

Copy link
Contributor

@mellis13 mellis13 left a comment

Choose a reason for hiding this comment

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

Just a couple of comments, but thank you for updating these docs. It is so much clearer on how to use the API.

bring your contribution into the codebases by testing it across a variety of
platforms and ensuring code quality.
bring your contribution into the codebases by testing it across a variety of
platforms and ensuring code quality.

- You will be credited as a co-author when the contribution is merged
Copy link
Contributor

Choose a reason for hiding this comment

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

Period at the end of this to match other formatting

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

client.run_script("normalizer", "normalize", inputs=["X_rand"], outputs=["X_norm"])
x_norm = client.get_tensor("X_norm")

Notice that the key ``"normalizer"`` represents the script containing the function (similar to
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mind adding more information here? I think it might be easier to understand if you provided more guidance during the 'Notice' sentence, possibly point out run_script() or specifically where the function and script are named, I might split the code up into parts as well -> describe set_function(), describe run_script()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added in-line comments to avoid breaking the code block. Let me know if it is more understandable now!

Copy link
Contributor

@mellis13 mellis13 left a comment

Choose a reason for hiding this comment

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

Couple of very small comments, otherwise I think when other reviewers' comments are addressed it should be good to merge.

@mellis13 mellis13 added the area: ML Issues related to SmartSim ML classes and utilities label Dec 12, 2023
Copy link
Collaborator Author

@al-rigazzi al-rigazzi left a comment

Choose a reason for hiding this comment

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

Added tabs and fixed conflicts in requirements-doc.txt afaict.

for language-specific details.

Using ML models on the DB
=========================
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep

client.run_script("normalizer", "normalize", inputs=["X_rand"], outputs=["X_norm"])
x_norm = client.get_tensor("X_norm")

Notice that the key ``"normalizer"`` represents the script containing the function (similar to
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added in-line comments to avoid breaking the code block. Let me know if it is more understandable now!

TensorFlow and Keras models <smartsim_tf_api>`: ``freeze_model`` and
``serialize_model``.

The method ``freeze_model`` is thought to be used in conjunction
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's skip the full source code, as the page will be already quite long. What do you think?

@al-rigazzi al-rigazzi requested a review from MattToast December 12, 2023 17:47
client.run_script("shifter", "shift_y_to_x", inputs=["X_rand", "Y_rand"], outputs=["Y_scaled"])
# Download output
y_scaled = client.get_tensor("Y_scaled")

Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a mini end summary here, something like Notice how I use the function Client.set_script_from_file() to upload the script to the DB, then call the script from within the DB via Client.run_script(), the output tensors are stored in the DB so I use get_tensor to retrieve

Copy link
Contributor

Choose a reason for hiding this comment

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

ermmm I see you actually have this in the code comments, maybe its actually fine

Copy link
Contributor

Choose a reason for hiding this comment

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

I am able to understand it and follow by reading the comments btw!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added a small recap anyhow!

Copy link
Contributor

@amandarichardsonn amandarichardsonn left a comment

Choose a reason for hiding this comment

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

These revisions you've made are awesome, I can clearly follow along and I get an understanding of what is coming before I read. As everyone says, this PR LGTM>>>>>>>>>>

@@ -46,6 +47,7 @@
sr_data_structures
sr_dataset_conversions
sr_runtime
sr_advanced_topics
Copy link
Contributor

Choose a reason for hiding this comment

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

nice catch!

@al-rigazzi al-rigazzi merged commit 3ee7794 into CrayLabs:develop Dec 13, 2023
@al-rigazzi al-rigazzi deleted the torchscript-docs branch December 13, 2023 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: docs Issues related to documentation area: ML Issues related to SmartSim ML classes and utilities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants