-
Notifications
You must be signed in to change notification settings - Fork 287
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
add __hash__
method to FlyteFile
to fix bug during interactive mode
#2853
Conversation
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2853 +/- ##
==========================================
- Coverage 45.53% 38.27% -7.26%
==========================================
Files 196 196
Lines 20418 20473 +55
Branches 2647 2650 +3
==========================================
- Hits 9298 7837 -1461
- Misses 10658 12427 +1769
+ Partials 462 209 -253 ☔ View full report in Codecov by Sentry. |
@Mecoli1219 did some digging around this and found that jupyter is checking to see if this is stdin, out, err. I think this is fine, but in the interest of rigor, i don't see a harm in including all the fields of the object (excluding the downloader function but including the downloaded bit) as part of the hash computation. what do you think @Mecoli1219? Also this change is not needed for the FlyteDirectory type right? That also has a |
I am not really sure, but I think that there wouldn't be a use case where there are two FlyteFiles in the same process that have the same path with different downloader functions. If there exists, that would be really weird. Let's say we create two FlyteFile:
The order of executing If the previous statement is right, we would only have two FlyteFiles with the same downloader function, and this also means that two FlyteFiles should be the same FlyteFile. However, it is possible that the downloaded bits are different in two instances (Just think that one calls So I don't think I'll include the downloaded bit here, but I am not entirely familiar with the usage of FlyteFile, this is just my current thought. |
I tried a number of operations and I couldn't produce any unexpected behavior. Yes, you are right, I should have used a more comprehensive hash function. |
This task will fail only in a notebook. It will not fail when running locally as a python script, running locally via pyflyte, or running remotely.
This can be fixed by adding the
__hash__
method toFlyteFile
. I am creating the hash from the serialized representation of theFlyteFile
.