Skip to content
Snippets Groups Projects

Some fixes to wrapper API

Closed Amir MOHAMMADI requested to merge wrapper-api2 into wrapper-api
1 unresolved thread

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
1 # see https://docs.python.org/3/library/pkgutil.html
2 from pkgutil import extend_path
3
4 __path__ = extend_path(__path__, __name__)
  • Hey @amohammadi, thanks for the MR.

    With respect to the transformers, I think the biggest difference between this MR and !185 (merged) is the way that you expanded the wrapped transformers.

    I've created one transformer per element (preprocessor, extractor, algorithm) and a wrap_bob_legacy that wraps this with checkpoint and sample wrappers (i think this should be the default behavior). You explicitly created one class for each combination (TransformerPreprocessor, SamplePreprocessor,CheckpointPreprocessor...). Do you want to keep that? I think give a lot of options confuses more than helps. Besides, you can create this on the fly if needed.

  • @tiago.pereira there is a lot of detail to take care of when you want to wrap legacy classes in SampleWrapper and CheckpointWrapper classes. For example:

    • to know for preprocessors that transform_extra_arguments=(("annotations", "annotations"),)
    • To know for extractors that transform_extra_arguments, fit_extra_arguments changes depending on the extractor
    • To know to pass the model_path to the extractor and not the CheckpointWrapper
    • And same things for algorithm.

    It's not easy to figure these options out. So, it's best to provide them already IMO.

  • Yeah, I know, but my functions take care of those cases.

    For instance, Wrapping a preprocessor

            wrap_preprocessor(
                PreprocesorTransformer(),
                features_dir="./",
                transform_extra_arguments=(("annotations", "annotations"),),
            ),

    Wrapping an extractor

            wrap_extractor(
                ExtractorTransformer(),
                features_dir="./"
            ),

    Wrapping an algorithm

            wrap_algorithm(
                AlgorithmTransformer(),
                features_dir="./",
                model_path="./xxx",
                fit_extra_arguments=(("subject", "y"),),
            ),

    In case you want to benefit from some default path parameters, you can use directly wrap_bob_legacy function.

            wrap_bob_legacy(
                BobBioPreprocesor(),
                base_path,
                transform_extra_arguments=(("annotations", "annotations"),),
            ),

    However, in case you know very well what you are doing, you can wrap your functions yourself with your stuff.

  • @tiago.pereira there is only one way to call these functions. If you leave it to the user, they will make errors. As I explained, when you wrap:

    • A preprocessor, the transform_extra_arguments=(("annotations", "annotations"),) argument is mandatory.
    • A extractor, the transform_extra_arguments = (("metadata", "metadata"),) argument is mandatory if they accept metadata
    • A extractor, the fit_extra_arguments = (("y", "subject"),) argument is mandatory if extractor.requires_training and extractor.split_training_data_by_client and It should not be provided otherwise.

    ... Leaving this to the user is not a good idea.

    Also, in the future, we may want to use these in different scenarios. So sometimes, I may just want a transformer, sometimes only a samplewrapper, and sometimes with checkpointing. So all these functions/classes will be used.

  • Ok, I'll integrate them in the other MR.

  • Thank you! I'm looking forward on working on the same base again.

  • Yeap, me too. I made several changes.

  • mentioned in merge request !185 (merged)

  • Please register or sign in to reply
    Loading