Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
MultiPartCopy with Sync Algorithm #4475
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
MultiPartCopy with Sync Algorithm #4475
Changes from 1 commit
344d26b
374c638
67d8ec8
2fa0503
30c2b91
ef57f14
c44acd2
297d1b6
18a5728
4554d34
c7f3f96
28c9186
97001cc
27240a9
ce73f62
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't seen this annotation before, I assume it's lifted from
aws s3 sync
. Can we use a simpler implementation without these fancy annotations? Unless they're absolutely necessary, I feel like they make maintainability more difficult.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya, this is just a really fancy way of implementing an if/else block. The true right way of doing this is through polymorphism where the incoming objects all implement a common interface (in this case, they all would define a method called
.format()
that you would be able to call). If that requires significant refactor, then if/else (or a case/switch in higher versions of python) is probably the better way to goThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the input I'll take a look at this. I wanted to take in a single input field that could be one of two types. Realistically, I can have two optional input fields and assert that at least one must be defined. Then I can branch my logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Ben on this, I believe this could be a good practice long-term to avoid bloating our functions with optional fields. I'd argue that
singledispatchmethod
achieves polymorphism in a functional rather than OOP style. Instead of having aFactory
that creates multiple classes that implementformat()
, Ben's implementation here cuts down on class boilerplate by method overloadingformat(input)
directly with single-line decorators. IMO it's confusing because it's a new paradigm we're not used to yet, not because it's a bad way to implement itThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a way of implementing the @singledispatch function above. I wanted the
.format
function to take in one of two params and perform different actions. Essentially if/else block but this was nicer with input typesThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.python.org/3/library/functools.html#functools.singledispatchmethod
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are fine with regular print statements?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Placeholder for now, I'll double check if we want to use logger (prob the case)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we rename,
hub_content_dependency_to_accessor_dict
or something to that effect? And can we put in another common module?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I use the instantiated
PublicModelDataAccessor
, but I can likely store that as a constant in the class and use that to map to the functionThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is for inference notebooks, no? Naming feels a little unclear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it's for the demo notebook. We can also store inference notebooks, but that is not found in metadata
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, why is the key
key = f"{framework}-notebooks/{self.model_specs.model_id}-inference.ipynb"
then?