trackio by pcuenca · Pull Request #3669

trackio by pcuenca · Pull Request #3669 · huggingface/accelerate

Not sure if this has been discussed before, feel free to disregard if not appropriate.

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

		accelerator = Accelerator(log_with="tensorboard", project_config=config)
		```

		### Using trackio

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems a bit out of place here. Maybe put it a bit higher up?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, it's the first place after the trackers are introduced with an example. Where would you put it?

Co-authored-by: Steven Liu <59462357+stevhliu@users.noreply.github.com>
Co-authored-by: Abubakar Abid <abubakar@huggingface.co>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice ! I think we should still create a new Tracker class for Trackio. See WandBTracker for example. This way ppl can track with trackio by passing log_with="trackio".

create a new Tracker

Cool, happy to do that in a followup PR! Should we merge this one for now?

I would say we should just do it in 1 PR, or you think it's too much? This PR is kindof skinny still, should be ok to add it.

It would be better to add in this PR @pcuenca !

		kwargs:
		Additional key word arguments passed along to the `trackio.log` method.
		"""
		self.run.log(values, **kwargs)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +289 to +290

		def is_trackio_available():
		return sys.version_info >= (3, 10) and _is_package_available("trackio")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trackio requires 3.10 but some of the CI tests run on 3.9, which took me a while to realize.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much! 👏

Co-authored-by: Steven Liu <59462357+stevhliu@users.noreply.github.com>

Not sure what's going on, tests passed yesterday now they don't, we just changed this.

Not sure what's going on, tests passed yesterday now they don't, we just changed this.

This is unrelated to your PR, tests are breaking all over the place, seems to be related to datasets==4.0.0 weird interaction with evaluate.load("glue", "grpc") which we use in many of our tests, I'm investigating a bit more and we'll lock to <=4.0.0 worst case.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this is looking good ! Can you also add a test just like what we have for wandb in WandBTrackingTest + run a full training to see if we get the expected plots. You can take inspiration from this PR #3605

Thanks for the review, @SunMarc!

I did not add that test because I saw that the wandb one has been disabled for 2 years, so I wasn't sure if it'd be useful work. I did mimic the _deferred_init one, which is active.

I'm off for two weeks, so anyone feel free to take this over if you are bored :) Otherwise I'll resume when I'm back; all I wanted was just mention trackio in the docs 😅.

I'm off for two weeks, so anyone feel free to take this over if you are bored :) Otherwise I'll resume when I'm back; all I wanted was just mention trackio in the docs 😅.

I'll quickly check that logging indeed works and merge this then !