Skip to content
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

Include a readable repr for ModelInfo #32

Merged
merged 3 commits into from
May 4, 2021
Merged

Include a readable repr for ModelInfo #32

merged 3 commits into from
May 4, 2021

Conversation

muellerzr
Copy link
Contributor

This includes the start of a better __repr__ for the ModelInfo class, started from this issue: #31

This is based on what I've started for the AdaptNLP

A few points of difference that could be done here is we could also programmatically include the tasks available for it, similar to what was done in AdaptNLP, but I guess the key information is what would be the best-case scenario for a model repr? Currently what I have lets it look like so:

Model Name: "gpt2", Tags: ['pytorch', 'tf', 't5'], Task: ['summarization']

For adapt I go a bit further (with the whole programmatic way I mentioned early) by checking what tags are also tasks and assigning them then.

The last thing is if we want something like:

Model Name: "gpt2-hybrid", Author: "username", Tags: ..., Task: ...

What are your thoughts?

cc: @LysandreJik

@LysandreJik
Copy link
Member

I think the proposition of making repr more understandable is great! Thinking about from a high-level perspective, I understand that the difference between __repr__ and __str__ is that the former is for developers (useful when debugging), should be unambiguous, while the latter is for end-users and supposed to be readable.

To that end, I'm wondering if it wouldn't be better to have a __repr__ that shows a bit more information, like the __dict__, while the __str__ outputs the current __repr__ output. What do you think?

@muellerzr
Copy link
Contributor Author

@LysandreJik I do like that idea, let me work on that this weekend and I'll get back to you with what I come up with in regards to a presentable format/strings

@muellerzr
Copy link
Contributor Author

muellerzr commented May 1, 2021

@LysandreJik apologies it took so long, had some final exams and projects that needed to get done!

Here's my adjustments I've made, the __repr__ now gives an output like so:

ModelInfo: {
	modelId: distilgpt2
	sha: None
	tags: ['pytorch', 'tf', 'tflite', 'rust', 'gpt2', 'lm-head', 'causal-lm', 'en', 'dataset:openwebtext', 'exbert', 'license:apache-2.0', 'text-generation']
	pipeline_tag: text-generation
	siblings: [<huggingface_hub.hf_api.ModelFile object at 0x7efdbba60090>, <huggingface_hub.hf_api.ModelFile object at 0x7efdbba600d0>, <huggingface_hub.hf_api.ModelFile object at 0x7efdbba60110>, <huggingface_hub.hf_api.ModelFile object at 0x7efdbba60150>, <huggingface_hub.hf_api.ModelFile object at 0x7efdbba60190>, <huggingface_hub.hf_api.ModelFile object at 0x7efdbba60210>, <huggingface_hub.hf_api.ModelFile object at 0x7efdbba60250>, <huggingface_hub.hf_api.ModelFile object at 0x7efdbba60290>, <huggingface_hub.hf_api.ModelFile object at 0x7efdbba602d0>, <huggingface_hub.hf_api.ModelFile object at 0x7efdbba601d0>]
	private: False
	key: 
}

Which I think is more aligned to what you were wanting, while the str has the original, IE:

Model Name: distilgpt2, Tags: ['pytorch', 'tf', 'tflite', 'rust', 'gpt2', 'lm-head', 'causal-lm', 'en', 'dataset:openwebtext', 'exbert', 'license:apache-2.0', 'text-generation'], Task: text-generation

Do let me know your thoughts please! :)

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks @muellerzr!

@LysandreJik
Copy link
Member

Before we merge, you seem to have code quality issues, could you run the following from the root of your clone?

pip install .[quality]
make style
make quality

@muellerzr
Copy link
Contributor Author

@LysandreJik it seems to have picked up an unrelated file with it, but I ran the code quality

@LysandreJik
Copy link
Member

Fantastic, thanks a lot for working on this!

@LysandreJik LysandreJik merged commit 67198a0 into huggingface:main May 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants