Initial pass at wandb Ludwig integration

[vanpelt]

This is initial pass at a basic integration of wandb with Ludwig. It currently mirrors tensorboard events, stores hyperparameters, syncs artifacts, and stores evaluation results.

I'm using the python "black" code formatter which seems to be clashing with some of the existing code. Is there a preferred formatter to use?

[CLAassistant]

All committers have signed the CLA.

[vanpelt]

Just tried to sign the CLA and it's blank...

[w4nderlust]

Just tried to sign the CLA and it's blank...

@briankhsieh can you please look into the CLA issue?

[w4nderlust]

This is initial pass at a basic integration of wandb with Ludwig. It currently mirrors tensorboard events, stores hyperparameters, syncs artifacts, and stores evaluation results.

I'm using the python "black" code formatter which seems to be clashing with some of the existing code. Is there a preferred formatter to use?

Thanks for this contribution, will look into it shortly.
As for formatter, we usually use PyCharm's one with the line length set to 79.

[bhenhsi]

It looks like cla-assistant.io is having intermittent issues. It worked for me this morning. Can you try again later?

[vanpelt]

@briankhsieh got it this time, just took a little while to load.

[vanpelt]

@w4nderlust I'm using VSCode, if you have any ideas on settings I'm all ears. I'll do some googling as well. Other than formatting, any thoughts on the pull?

[w4nderlust]

@vanpelt sorry for the delay on this, was out for conferences. Will look into it by end of day Friday.

[w4nderlust]

Please format this on 4 lines:

save_csv(
    csv_filename.format(output_field, output_type),
    values
)

[w4nderlust]

This looks longer than 80 columns, please split accordingly

[w4nderlust]

I'm really sorry for the long delay @vanpelt , I had some personal issues that kept me away from working on Ludwig, now I'm back. The PR looks good to me as a starting point, just added a couple formatting suggestions. Regarding VSCode, I don't use it, but likely you can set the linter to 80 columns somehow (note sure this is still valid but https://stackoverflow.com/questions/29968499/vertical-rulers-in-visual-studio-code ). Do you prefer if I merge it now or do you want to keep on working on it adding the missing features?

[vanpelt]

Hey @w4nderlust no worries. Let me take a pass at getting formatting setup properly and address your comments. I'll have something shortly.

[vanpelt]

Ok, @w4nderlust addressed your feedback. Let me know if I missed anything, I'm still seeing DeepSource issues. Would love to get my linter / formatter setup correctly in vscode but not sure how best to do it.

[w4nderlust]

I looked at the deepsource complaints. They are msotly related with global and import.
Doing import more than one time in multiple threads would be a problem?
If not, maybe you could do the same thing I'm doing with the horovod import here: https://github.com/uber/ludwig/blob/master/ludwig/models/model.py#L91-L94 Assigning the imported library as a reference variable in self, so that in the rest of the class of the function you just call self.wandb.
It would look like:

try:
    import wandb
    wandb_obj = Wandb()
    setattr(wandb_obj, 'wandb', wandb)
    return wndb_obj

What do you think?

[vanpelt]

Sorry for the delay on this, I'll hopefully get to it this week.

[vanpelt]

Ok @w4nderlust finally got to this. Let me know if you need any other changes!

[vanpelt]

@w4nderlust Ping

[vanpelt]

Hey @w4nderlust any updates here?

[w4nderlust]

Checked it out, lgtm, will test it later this week and it everything works fine as expected will merge. please give me some instructions for testing, like an example command or something like that ;)

[vanpelt]

Awesome! To test it, create a wandb account here: https://app.wandb.ai then run wandb login on your machine. Then just run a Ludwig job with the —wandb flag.

[vanpelt]

@w4nderlust any updates here?

[vanpelt]

@w4nderlust I'm sure the holidays are hectic. Would love to get this in, do you have an eta?

[w4nderlust]

Hey @w4nderlust I'm having a colleague try the TF2 integration and add a test. Hopefully we'll have something next week.

Thank you, sounds good! Consider the TF2 integration is not there yet, the test would be there to avoid problems when we will do it.

[borisdayma]

The comments should now be addressed.
I just need to do a few manual tests and add an integration test for wandb.

Quick questions:

• We currently rely on syncing tensorboard data but it seems to be missing validation/test logs. Is that correct? If so we will just use the train_epoch_end method to log them to W&B
• It seems our predict_end is never called. I guess it is not used during training and only for inference on trained models. If so then we will remove the call to predict_end and you don't need to add it to the docs @dsblank
• DeepSource does not like a few things and also wants to convert back the methods to static since we don't use self. Can we ignore it?

[w4nderlust]

The comments should now be addressed.
I just need to do a few manual tests and add an integration test for wandb.

Thank you!

Quick questions:

• We currently rely on syncing tensorboard data but it seems to be missing validation/test logs. Is that correct? If so we will just use the train_epoch_end method to log them to W&B

Yes Currently the tensorboard contains only the batch level measures (that are only about the training set) and none of the epoch level measures we display in the cli. There is an open PR for that #571 that will be merged soon.

• It seems our predict_end is never called. I guess it is not used during training and only for inference on trained models. If so then we will remove the call to predict_end and you don't need to add it to the docs @dsblank

That's fine. if other integrations need it we will add it, but if you don't need it we can do without for now.

• DeepSource does not like a few things and also wants to convert back the methods to static since we don't use self. Can we ignore it?

It's fine to ignore them, in particular if they are warnings. I would say try to address as many as you can, but only do it if it makes sense. The static method complaint can be ignored for instance.

[borisdayma]

Except for formatting issues everything should be working.

Here is an example run: https://app.wandb.ai/borisd13/ludwig_mnist/runs/qzu4b5qq?workspace=user-borisd13

Whenever the other metrics are added to tensorboard they should automatically become in sync with W&B.

Let me know if I need to change anything!

[w4nderlust]

I would add name=model_name to the init parameters.

[borisdayma]

Good idea, I was not aware of what model_name was used for initially but now it makes sense to use it as the W&B run name

[w4nderlust]

@borisdayma tested it and everything looks fine. Added just one little comment about the model name, as at the moment one gets names that are auto-generated by wandb I believe like "dandy-donkey-2". Once that is done, I'll merge.

[borisdayma]

I added it and the runs are now named per model_name, see example:
https://app.wandb.ai/borisd13/ludwig_mnist/runs/vsf46v66?workspace=user-borisd13

Once PR #571 is merged, the visualization will be more useful as validation metrics should also be logged.

[w4nderlust]

@borisdayma thank you, will merge that one first, test again ,and then merge this. I Don't expect any problem to happen, but just want to be sure.

[w4nderlust]

THanks a lot for your work and your patience. Sorry if it took me longer than I expected to get to this, but in the end we merged it :)

[borisdayma]

Thanks, I'll be playing with it!

[CharlyWargnier]

Thanks for that great integration guys! Is it now fully working?

[w4nderlust]

Yes!