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

Refactor/document encode #2160

Merged
merged 12 commits into from
Jun 13, 2024
Merged

Refactor/document encode #2160

merged 12 commits into from
Jun 13, 2024

Conversation

jieguangzhou
Copy link
Collaborator

Description

Related Issues

Checklist

  • Is this code covered by new or existing unit tests or integration tests?
  • Did you run make unit_testing and make integration-testing successfully?
  • Do new classes, functions, methods and parameters all have docstrings?
  • Were existing docstrings updated, if necessary?
  • Was external documentation updated, if necessary?

Additional Notes or Comments

@jieguangzhou jieguangzhou force-pushed the refactor/document-encode branch 2 times, most recently from 1908c62 to f481bdd Compare June 12, 2024 16:29
Comment on lines 416 to 419
# TODO: How about the lazy loading of the artifact?
# Should this function return a callback function for getting the artifact?
def _get_artifact(db, path):
return db.artifact_store.get_bytes(path)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@blythed The current logic for fetching bytes from the database is handled in the artifact, which parses the reference and then retrieves the bytes. Please confirm.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes I like the callback approach. We could use this approach also for doing downloadable artifacts at some point.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So you would "forward" a getter to the Artifact

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

Comment on lines 545 to 549
# TODO: Need to sort out the initialization of the artifact
# 1. Use x(bytes) and schema for decoding. (bytes from _blobs)
# 2. Use x(reference) and schema for decoding.
# 3. use _blob(reference) for decoding. (bytes from _blobs)
# 4. use _blob(bytes) for decoding
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@blythed Please confirm.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that x is the actual decoded thing (like PIL.Image) and blob is the bytes. blob can either be a reference or pure bytes, depending on the class.

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

@jieguangzhou jieguangzhou force-pushed the refactor/document-encode branch 5 times, most recently from 64579e7 to fe2afe6 Compare June 13, 2024 13:01
@blythed blythed merged commit f4076bb into main Jun 13, 2024
3 checks passed
@blythed blythed deleted the refactor/document-encode branch June 13, 2024 14:02
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.

2 participants