feat: automatic model card generation on save

[plaguss]

Description

This PR adds the option to generate a model card from the ArgillaTrainer when calling the save method.
Closes #3634

Type of change

(Please delete options that are not relevant. Remember to title the PR according to the type of change)

• New feature (non-breaking change which adds functionality)
• Refactor (change restructuring the codebase without changing functionality)
• Improvement (change adding some improvement to an existing functionality)

How Has This Been Tested

(Please describe the tests that you ran to verify your changes. And ideally, reference tests)

• tests/integration/client/feedback/integrations/huggingface/test_model_card.py

Checklist

• I added relevant documentation
• I followed the style guidelines of this project
• I did a self-review of my code
• I made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• I filled out the contributor form (see text above)
• I have added relevant notes to the CHANGELOG.md file (See https://keepachangelog.com/)

[davidberenstein1957]

Hi @plaguss,

This is looking good already. Some remarks.

For a separate PR:
[ ] I think we should add a push_to_huggingface method to each of the trainers. Here we can nicely include the cards.
[ ] Also, we need a call or predict method for each for each of the trainers.
I believe there are some issues on GitHub but otherwise feel free to create them.

For this PR:
[ ] Missing some tests.
[ ] missing some docs.
[ ] We should add model_card_data to the from argilla.client.feedback.training ABC base class.

model_card_data might be renamed to get_model_card_data
maybe integrations/huggingface/model_card is more explicit?

[davidberenstein1957]

is there a way to get these arguments too?

[davidberenstein1957]

Don't spend too much time on this.

[plaguss]

I'm working on this right now, but its so draft to push the commits yet. They are a bit trickier depending on the model. For example, transformers is almost done as the inner trainer_kwargs is relatively simple, but in the trl framework it will take some work to nicely print config=PPOConfig(batch_size=1, ppo_epochs=1) I think. I'm working on it, if I see that it needs too much effort I'll let you know 😃.

[plaguss]

With the current implementation we cannot write the arguments passed by the user to update_config in the following cases:

• spacy: it jdoesn't work due to how the training configuration is defined with nested dicts, the (naive) function _updated_arguments which grabs the user arguments cannot currently deal with nested dicts. For example, the default configuration for spacy is:

{
    'dev_corpus': 'corpora.dev',
    'train_corpus': 'corpora.train',
    'batcher': {
        '@batchers': 'spacy.batch_by_words.v1',
        'discard_oversize': False,
        'tolerance': 0.2,
        'size': {'@schedules': 'compounding.v1', 'start': 100, 'stop': 1000, 'compound': 1.001, 't': 0.0},
        'get_length': None
    },
    'optimizer': {'@optimizers': 'Adam.v1', 'beta1': 0.9, 'beta2': 0.999, 'L2_is_weight_decay': True, 'L2': 0.01, 'grad_clip': 1.0, 'use_averages': False, 'eps': 1e-08, 'learn_rate': 0.001},
    'seed': '${system.seed}',
    'gpu_allocator': '${system.gpu_allocator}',
    'dropout': 0.1,
    'accumulate_gradient': 1,
    'patience': 1600,
    'max_epochs': 0,
    'max_steps': 20000,
    'eval_frequency': 200,
    'score_weights': {
        'cats_score': 1.0,
        'cats_score_desc': None,
        'cats_micro_p': None,
        'cats_micro_r': None,
        'cats_micro_f': None,
        'cats_macro_p': None,
        'cats_macro_r': None,
        'cats_macro_f': None,
        'cats_macro_auc': None,
        'cats_f_per_type': None
    },
    'frozen_components': [],
    'annotating_components': [],
    'before_to_disk': None,
    'before_update': None,
    'logger': {'@loggers': 'spacy.ConsoleLogger.v1', 'progress_bar': False}
}

• trl (when using PPO only), the internal configuration makes it harder to print a nice representation for this case, mainly due to PPOConfig, so I prefer to let this for the.

[plaguss]

Hi @plaguss,

This is looking good already. Some remarks.

For a separate PR:
[ ] I think we should add a push_to_huggingface method to each of the trainers. Here we can nicely include the cards.
[ ] Also, we need a call or predict method for each for each of the trainers. I believe there are some issues on GitHub but otherwise feel free to create them.

For the moment I'm generating the call to the predict method if it exists for the framework, and a sample call to the underlying framework (extracted from the docs) for the models that don't have the predict implemented. But yeah, it would be nice to have the predict method available at least for testing the model.

For this PR:
[ ] Missing some tests.
[ ] missing some docs.
[ ] We should add model_card_data to the from argilla.client.feedback.training ABC base class.

Working on the tests! I didn't forgot them, I'm still refining them. I wasn't sure of adding it to the ABC base class, but will do it.
Of course to the docs.

model_card_data might be renamed to get_model_card_data
maybe integrations/huggingface/model_card is more explicit?

Agree, I'll do it.

I've found a decent amount of details, but it's already taking form. As soon as I got more time I'll let it ready for review 😄

---

Update:

• model_card_data added to from argilla.client.feedback.training ABC base class.
• model_card_data renamed to get_model_card_data.
• maybe integrations/huggingface/model_card is more explicit? -> yes! done

[plaguss]

Hello @davidberenstein1957,
When you can, please take a look at the tests to see if the approach looks okay to you. They are currently only checking the code snippet generated to see if it matches. Also, we need to train the models in order to generate the model cards, which adds a bit of overhead (around 40 seconds currently). We could test this where we are testing the behavior of the to speed up the tests a bit, but I think its better to have them separated.
And a question for when I tackle the docs. Where would you like me to add the new feature? It needs a short guide on how to add variables to the ArgillaTrainer that would be written on the model card, and some example. Thank you!

[davidberenstein1957]

Hi @plaguss, thanks for the iteration 👍

I think we can also just call the get_model_card or generate_model_card after initializing the Trainer? Alternatively, we could do some test-mocking to avoid starting the actual training process. Ultimately, we would be able to try to just test the specific function. I agree it is best to keep the tests separate.

The tests look fine, I think these need to be fixtures and can be added to aconftest.py. Similarly, we can retry using the formatting_func to from the other places in the tests where we actually train the models. We could also use a IOBuffer as output directory and avoid saving the meta file to an actual output directory. Also, this logic is copied for all the different frameworks.

In terms of the docs, I would say adding it here would be great.

[plaguss]

• The tests have been updated to avoid calling the trainer, much faster now, and the same results, no need to mock here (40s -> 7s)
• All the patterns to check for the tests have been moved to conftest.py.
• Regarding the IOBuffer, I'm not so sure what you mean. Anyway, now all the file creation is done inside a TemporaryDirectory

with TemporaryDirectory() as tmpdirname:
    content = trainer.generate_model_card(tmpdirname)

I think it's a good idea to move all formatting_func to a common module (maybe here instead of a conftest.py. I think makes sense as separate functions more than fixtures, and that way we don't need to update the checks for the model cards, as internally we are reading the source code). But maybe that can be done in a different PR? I can open an issue and get it done after this one if it's ok.

[davidberenstein1957]

https://www.digitalocean.com/community/tutorials/python-io-bytesio-stringio

and a separate PR would be great. Also, we can think of creating common formattinng_funcas part of #3833

[plaguss]

We could add more data to the model card (metrics from the models should be there), but I think that it works in the current state, we can always make an upgrade 😃.

[davidberenstein1957]

We could add more data to the model card (metrics from the models should be there), but I think that it works in the current state, we can always make an upgrade 😃.

I agree

[plaguss]

Hi @davidberenstein1957 do you know about the errors on the unit tests? They seem unrelated, I don't know if I missed something

[davidberenstein1957]

@plaguss, they are unrelated and we have been having them for a while without a clear solution besides re-running them.