-
-
Notifications
You must be signed in to change notification settings - Fork 280
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 CTrL integration #561
add CTrL integration #561
Conversation
Pull Request Test Coverage Report for Build 1649217012
💛 - Coveralls |
Hi @TomVeniat! Thanks for contributing to Avalanche! Let us know when we can start to review this and please add some comments in the PR description so that we can understand the logic behind it! |
Hi @vlomonaco, |
Hi Tom, you can add a comprehensive example on its usage in the |
Maybe a better place for a notebook would be outside of avalanche's documentation, in our colab repository. We have a collection of notebooks and continual learning examples there. What do you think? |
I think that only exposing the "classic" streams is a good starting point, with a simple script in the About the testing, is there a way to add some information about each experience in a |
Yes, it makes sense, thanks! :)
You can check the "classes_in_experience" attribute which is populated automatically. As for tasks descriptors, unfortunately I'm not sure we currently support them as |
I'm afraid that @TomVeniat you need |
@TomVeniat any update on this? |
Hi @vlomonaco, |
Yes of course, thanks @TomVeniat! :) Let us know if you need any help! |
Oh no! It seems there are some PEP8 errors! 😕
|
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.
Hi Tom, apart from the comments I left in ctrl.py
and simple_ctrl.py
, can you add ctrl in the __init__.py
file in avalanche/benchmarks/classic
(following the alphabetical order of modules)?
Thank you @lrzpellegrini for the review, I'll make the suggested changes. The only thing missing for the full integration of CTrL is a way to handle longer streams. The current integration starts with loading all experiences as I will also open a new issue for the task-level cross-validation, the solution I had isn't that straightforward to implement so Id' like your input on it to make sure it fits nicely with the framework. |
Alas, in the current state this is not possible. However, this is a feature we already have in our backlog (see #600).
Yes, this is already doable. Have a look at:
paths_benchmark ). That helper accepts a list of files for each experience and it also supports custom streams (like the validation one). In that case, the best solution is to write any generated data to a temporary folder or some other kind of "working directory".
Avalanche doesn't currently support directly loading tensor(s) describing a whole experience directly from disk, but I think I can easily implement it if needed!
Yes, a separate issue seems the best solution. For that feature we'll need some support from @AntonioCarta on the strategy-side. |
I started the implementation following this strategy but It seems that there is no way to give the correct transform to each experience. The |
Yes, if a different transformation is needed for each experience, then |
Hi Tom, I recently implemented a minimal support for lazily generated streams, see #671. That approach may make things easier on your side! There is also some memory optimization tuning that can be used to prevent memory usage from exploding by dropping references to previous experiences (I still have to document it decently). That PR still has to be merged, but it should be a matter of hours. |
Thanks for the update! I should have posted here but I went the write to disk + |
Oh no! It seems there are some PEP8 errors! 😕
|
Oh no! It seems there are some PEP8 errors! 😕
|
It seems like the memory consumption is still too high for the long stream, I will need to make some changes on the ctrl package side to make it more efficient (I think that I keep a reference to all generated tasks so far during the generation process). |
@lrzpellegrini Any idea of what happened with the python 3.9 tests setup? |
@TomVeniat This seems to be the error:
This is from the I got the same error when running on I have a fix with which I am working locally, and to integrate that, you need to merge the PR I raised at facebookresearch/CTrLBenchmark. |
Thanks @ashok-arjun for this! :) |
Thanks @ashok-arjun, your PR is live! :) |
@lrzpellegrini Any idea why these tests aren't passing? I am not able to figure this out. |
Hi @ashok-arjun, is your repo in sync with avalanche master branch? If not, can you please update it (there may be conflicts to be solved)? |
@AndreaCossu Actually it is @TomVeniat 's branch that must be checked for conflicts, since he has made the PR :) |
The error related to the unit test failures is |
Thanks @ashok-arjun for looking into this issue !
It seems the error remains the same, with the updates of |
I added the |
Everything seems in order to me! |
Draft for #377