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

integrations: move callbacks/loggers to frameworks #221

Closed
daavoo opened this issue Feb 14, 2022 · 8 comments
Closed

integrations: move callbacks/loggers to frameworks #221

daavoo opened this issue Feb 14, 2022 · 8 comments
Labels
A: frameworks Area: ML Framework integration maintenance p1-important Include in the next sprint

Comments

@daavoo
Copy link
Contributor

daavoo commented Feb 14, 2022

@daavoo I think that this is quite a challenge for us to decide.

  1. On one hand we don't want to restrict the dependencies, because that could interfere with users workspace, and dvclive is probably not that important to force user to install particular version of the DL framework.

  2. On the other hand, we need to maintain our project and the wide range of supported frameworks makes it hard to be able to know which versions of particular frameworks we support. Solution to that would be restricting the supported versions, but that contradicts with 1. We can discuss this on our next meeting.

Originally posted by @pared in #220 (comment)

@daavoo daavoo added A: frameworks Area: ML Framework integration discussion requires active participation to reach a conclusion labels Feb 14, 2022
@daavoo
Copy link
Contributor Author

daavoo commented Feb 14, 2022

Another option would be to try to maintain integrations compatible across different versions by adding conditionals in our code. For example (#219):

        if self.model_file:
            if catalyst.version <= 0.22.0:
                checkpoint = runner.engine.pack_checkpoint(
                     model=runner.model,
                    criterion=runner.criterion,
                    optimizer=runner.optimizer,
                    scheduler=runner.scheduler,
                )
            else:
                # code compatible with >= 0.22.0
                . . .

This could potentially become hard to maintain

@daavoo daavoo mentioned this issue Sep 26, 2022
13 tasks
@daavoo
Copy link
Contributor Author

daavoo commented Sep 28, 2022

We should get rid of integrations and transfer to the framework itself when possible. As mentioned in the first point of #223

daavoo added a commit that referenced this issue Sep 28, 2022
We don't own ML framework dependencies and it is ML framework responsability to run the safety checks.

We will eventually move framework integrations to the framework itself #221
@daavoo
Copy link
Contributor Author

daavoo commented Sep 28, 2022

After #220 we realized that there is no easy way to maintain different integrations - on one hand, dvclive is not that important to make users specific version of a particular framework. On the other, just having non-restricted versions of frameworks in our setup is just asking for trouble (again #220). It seems that quite a popular approach is to have the integrations not in the logging tool itself, but rather in the framework (Tensorboard x MLFlow example). But this only makes sense once the API is more or less defined. In our case, we are still developing the functionalities, and trying to do that might cause maintenance problems (not only for us). For now, we will take the approach of restricting the minimal or maximal version of a particular integrated library. Once we have the dvclive API defined, we will consider how we want to proceed.

Originally Posted by @pared

daavoo added a commit that referenced this issue Sep 28, 2022
We don't own ML framework dependencies and it is ML framework responsability to run the safety checks.

We will eventually move framework integrations to the framework itself #221
@dberenbaum dberenbaum changed the title integrations: How to manage versions integrations: move callbacks/loggers to frameworks Mar 3, 2023
@dberenbaum dberenbaum added p2-medium and removed discussion requires active participation to reach a conclusion labels Mar 3, 2023
@dberenbaum
Copy link
Contributor

Would this require a major version bump? I think it's time we need to think about doing this.

@daavoo
Copy link
Contributor Author

daavoo commented Mar 3, 2023

Would this require a major version bump? I think it's time we need to think about doing this.

🤔 Unless we do some hacky stuff to keep the imports working with the same syntax as today, yes (probably not worth the effort)

@dberenbaum dberenbaum mentioned this issue Mar 3, 2023
@dberenbaum dberenbaum added p1-important Include in the next sprint and removed p2-medium labels Oct 11, 2023
@dberenbaum
Copy link
Contributor

dberenbaum commented Nov 7, 2023

Opened huggingface/transformers#27352 for hf transformers

@dberenbaum
Copy link
Contributor

Opened huggingface/accelerate#2139 for hf accelerate

@dberenbaum
Copy link
Contributor

Lightning no longer accepts new callbacks 😦. Wish I had seen this before I wrote the PR 😅 :

It was decided a while ago that we no longer add new experiment tracking frameworks to the core of Lightning due to maintenance overhead.

Lightning-AI/pytorch-lightning#14137 (comment)

Once the huggingface callbacks are released and documented, we can lower the priority on this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: frameworks Area: ML Framework integration maintenance p1-important Include in the next sprint
Projects
None yet
Development

No branches or pull requests

2 participants