Ignite: TrainsSaver requires TrainsLogger but doesn't use it

[achigeor]

❓ Questions/Help/Support

Hi,

I'm opening this issue as a question, because I'm not sure if it's an artifact or it has an intended purpose that I don't understand.

The new trains support introduced TrainsSaver together with TrainsLogger. It seems that to initialize the TrainsSaver object, you're required to pass a TrainsLogger logger (https://github.com/pytorch/ignite/blob/master/ignite/contrib/handlers/trains_logger.py#L610).
However, this logger is not used anywhere. Why is it required?

I would expect to be able to just do

Checkpoint(to_save, TrainsSaver(output_uri=output_uri), 'epoch', n_saved=n_saved,
                       global_step_transform=global_step_from_engine(trainer))

[sdesrozis]

@achigeor Thank you for this question.

That needs to be confirmed by @jkhenning but I think the TrainLogger is mandatory to have the stream opened to the cloud trains server. Indeed, TrainLogger not directly used but ensures that the trains session is created (and not closed). @achigeor Does it make sense ?

HTH

[jkhenning]

Hi @achigeor,

@sdesrozis is quite right 😄

Just to add some details, In order to use Trains, you must create an experiment session and work with a remote Trains Server (your own installed server, for example). Since initializing a Trains experiment might require various parameters (such as name, project etc.) and since TrainsLogger already provides this functionality, duplicating it in the TrainsSaver seemed unnecessary. It also seems like a pattern used with other loggers, and serves the purpose of further enhancing the saver later on, if required.

[achigeor]

Thanks for the clarifications @jkhenning and @sdesrozis!
Wouldn't the same functionality be achieved by checking if a task is running and if not throw an error (or warning and create a task ad hoc?)
We do use trains in our project! However, we take care of handling the tasks and logging ourselves, so in order to use the DiskSaver I end up just passing an empty logger object (trains is clever enough to not init a new task but use the current one that runs).

[sdesrozis]

It makes sense to me. @jkhenning your thoughts ?

[jkhenning]

@achigeor @sdesrozis What do you think about using a default value of None for the logger argument - this will still be a hint for non-experienced users that a logger is the quickest way to get Trains working in this context, but will allow more advanced users who already initialized Trains to skip it. Of course, if no logger is passed and Trains was not initialized, a UsageError will be raised.
Makes sense?

[sdesrozis]

@jkhenning if we want to handle remote sessions opened by users outside ignite loggers (and be interoperable), we should do following your idea 👍🏻

[achigeor]

@jkhenning yeap makes a lot of sense, this covers all usecases for advanced as well as new users as you said :+1:

[vfdev-5]

Anyone would like to send a PR with this minor fix ?

[jkhenning]

Created https://github.com/pytorch/ignite/pull/1043 😄