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

Data Manager Documentation Update #951

Merged
merged 38 commits into from
Jul 19, 2024
Merged

Data Manager Documentation Update #951

merged 38 commits into from
Jul 19, 2024

Conversation

jspeerless
Copy link
Collaborator

Citrine Python PR

Description

Updates to our documentation to account for changes brought on by the Data Manager feature.
Includes changes to remove inaccurate docs and code examples after the migration to teams and up to 3.0 as well.
Note that I mostly focused on directing users to the correct endpoints in the Data Manager world and remove things that were just plain wrong, not completely overhaul our docs (which might be a good use of time at some point).

PR Type:

  • Breaking change (fix or feature that would cause existing functionality to change)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Maintenance (non-breaking change to assist developers)

Adherence to team decisions

  • I have added tests for 100% coverage
  • I have written Numpy-style docstrings for every method and class.
  • I have communicated the downstream consequences of the PR to others.
  • I have bumped the version in __version__.py

Copy link
Collaborator

@kroenlein kroenlein left a comment

Choose a reason for hiding this comment

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

Markdown formatting standard at Citrine is one sentence per line, and breaking that line into individual clauses across lines if that one line gets too long. By doing that, you avoid confusing diffs triggered when words move past an 80-character threshold.

These comments change no content, and only apply the above rule.

jspeerless and others added 14 commits July 19, 2024 11:35
Co-authored-by: Ken Kroenlein <51962276+kroenlein@users.noreply.github.com>
Co-authored-by: Ken Kroenlein <51962276+kroenlein@users.noreply.github.com>
Co-authored-by: Ken Kroenlein <51962276+kroenlein@users.noreply.github.com>
Co-authored-by: Ken Kroenlein <51962276+kroenlein@users.noreply.github.com>
Co-authored-by: Ken Kroenlein <51962276+kroenlein@users.noreply.github.com>
Co-authored-by: Ken Kroenlein <51962276+kroenlein@users.noreply.github.com>
Co-authored-by: Ken Kroenlein <51962276+kroenlein@users.noreply.github.com>
Co-authored-by: Ken Kroenlein <51962276+kroenlein@users.noreply.github.com>
Co-authored-by: Ken Kroenlein <51962276+kroenlein@users.noreply.github.com>
Co-authored-by: Ken Kroenlein <51962276+kroenlein@users.noreply.github.com>
Co-authored-by: Ken Kroenlein <51962276+kroenlein@users.noreply.github.com>
Co-authored-by: Ken Kroenlein <51962276+kroenlein@users.noreply.github.com>
Co-authored-by: Ken Kroenlein <51962276+kroenlein@users.noreply.github.com>
Co-authored-by: Ken Kroenlein <51962276+kroenlein@users.noreply.github.com>
@jspeerless jspeerless requested a review from kroenlein July 19, 2024 15:40
Copy link
Collaborator

@kroenlein kroenlein left a comment

Choose a reason for hiding this comment

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

Mostly good. Some suggested changes, plus the following required updates to the indexes (deploy will fail b/c of build warnings):

  1. You deleted molecular_generation.rst, but you didn't delete the reference to it from index.rst.
  2. You deleted team_management_migration.rst, but you didn't delete the reference to it from FAQ/index.rst.
  3. You didn't add data_manager_migration to FAQ/index.rst.

Comment on lines 112 to 115

# Doing any operations (updating, deleting, dumping, etc.) to Datasets or GEMD Assets via a
# Project collection
project.[datasets/gemd_objects]....
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should never have been able to do this along a project-based route for GEMD objects. It always required a dataset (maybe delete was an exception? Definitely true for update). In the past, that returned a simple error. @pacdaemon and @anoto-moniz might have accidentally made something broken when they added their deprecations there.

Suggested change
# Doing any operations (updating, deleting, dumping, etc.) to Datasets or GEMD Assets via a
# Project collection
project.[datasets/gemd_objects]....
# Doing any operations (updating, deleting, etc.) to Datasets via a Project collection
project.datasets.delete(uid)

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 thought the actions were just read-only if only the project was reference. E.g. .list() or .get() would work, but not register or update

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed, though I used "update" as an example since we use "delete before. Still not sure about whether or not we allow update or other things from the team-level endpoints.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@kroenlein To clarify, is there something in the SDK code that makes you think something's broken? Or are you just saying this line of the documentation was wrong?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I ran it locally and got a giant splat of exception handling that did not indicate the real problem. However, I then ran the same code with v3.2.11 and got the same error, so if we have more elegant handling in the past, that was last in a previous release.

jspeerless and others added 12 commits July 19, 2024 13:25
Co-authored-by: Ken Kroenlein <51962276+kroenlein@users.noreply.github.com>
Co-authored-by: Ken Kroenlein <51962276+kroenlein@users.noreply.github.com>
Co-authored-by: Ken Kroenlein <51962276+kroenlein@users.noreply.github.com>
Co-authored-by: Ken Kroenlein <51962276+kroenlein@users.noreply.github.com>
Co-authored-by: Ken Kroenlein <51962276+kroenlein@users.noreply.github.com>
Co-authored-by: Ken Kroenlein <51962276+kroenlein@users.noreply.github.com>
Co-authored-by: Ken Kroenlein <51962276+kroenlein@users.noreply.github.com>
Co-authored-by: Ken Kroenlein <51962276+kroenlein@users.noreply.github.com>
@jspeerless jspeerless requested a review from kroenlein July 19, 2024 18:50
@jspeerless jspeerless merged commit 7c3d98e into main Jul 19, 2024
16 checks passed
@jspeerless jspeerless deleted the data-manager-docs branch July 19, 2024 19:55
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