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

Support decorating instance methods of classes with path attributes #58

Open
jwodder opened this issue Dec 17, 2021 · 6 comments · May be fixed by #59
Open

Support decorating instance methods of classes with path attributes #58

jwodder opened this issue Dec 17, 2021 · 6 comments · May be fixed by #59

Comments

@jwodder
Copy link
Member

jwodder commented Dec 17, 2021

Prompted by dandi/dandi-cli#852 (comment)

@jwodder jwodder changed the title Support decorating instance methods Support decorating instance methods of classes with path attributes Dec 17, 2021
@yarikoptic
Copy link
Member

yarikoptic commented Dec 17, 2021

answering question to #44 (comment) (which is now closed) here

@yarikoptic So what should #58 do about resolving path attributes when decorating methods?

I am thinking about something along the lines of abstracting away "fingerprinters" so we could generalize to not only path and not only the first arg of the function, but also to attributes etc. Inspection of signature to figure out which arguments we need to deal with is to be only once so it should not have notable performance hit IMHO.

  1. Define base class Fingerprinter which will be subclassed for File and Directory and where to _get_file_fingerprint and _get_dir_fingerprint would fold into along with the modified_in_window to be called at the end of them to return None if "too early"
  2. PathFingerprinter adapter will switch between file and dir fingerprinter depending on what the value (path) is (directory or not)
  3. generalize memoize_path into memoize_params (or any other name you like) which would also take an option (so more of functions nesting, uff) e.g. params: dict[str, Fingerprinter] to specify which fingerprinter to use for the args needing special handling. And add ability to specify access to class instance attributes
    params={
      "path": PathFingerprinter,
      "self.path": PathFingerprinter,  # here due to `self.` take corresponding attribute of the bound instance 
    }

edit 1: or make it more explicit by separate two options: args and params of the same kind.
4. arguments (not self. attributes) which went through "transformation" will be ignored in the calls to joblib like we did for the path. Their Fingerprinter should provide desired value as part of the returned fingerprint (e.g. resolved path as you @jwodder did in #45 IIRC)

@jwodder
Copy link
Member Author

jwodder commented Dec 17, 2021

@yarikoptic

  • Why would a user want to specify a fingerprinter other than PathFingerprinter? If we want to support fingerprinting of multiple path arguments, wouldn't it be simpler to take a list or lists of argument & attribute names?
  • So if an instance attribute is a fingerprinted path, are you saying that the unresolved path should continue to be part of the function arguments and thus the cache key, meaning that referring to the same file with two different paths won't give you the benefit of caching?

@yarikoptic
Copy link
Member

  • I might be over engineering, but who knows - we might use it for http URLs and fingerprint size/mtime/etag this avoiding redownload. Proceed as you see fit - we could generalize later
  • no, we will ignore original path as we do now, and fingerprinter will resolve and add resolved path into the invocation signature. So if different paths (relative or symlinks) point to the same location, it should use cached value since resolved path would be the same. Or did I miss the point?

@jwodder
Copy link
Member Author

jwodder commented Dec 17, 2021

@yarikoptic

no, we will ignore original path as we do now

How? The instance needs to be passed to the instance method, and if we tell joblib to ignore "self", then all non-path attributes will be ignored by joblib as well.

@yarikoptic
Copy link
Member

Ah, good point. Let me think about it.

@yarikoptic
Copy link
Member

yarikoptic commented Dec 17, 2021

Aha, I think it is manageable although fragile: we can also add argument to list attributes to be added to the fingerprint (if needed). Sure we wouldn't capture full state but I expect it to be applied cautiously ;-)

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 a pull request may close this issue.

2 participants