Skip to content

Conversation

@osanseviero
Copy link
Contributor

@osanseviero osanseviero commented Mar 30, 2022

First part of 4 for #83
Internal change at https://github.com/huggingface/moon-landing/pull/2514

This PR changes how we define the types into a way that is better structured, more flexible, and give us more control (e.g. define if we want to show a type just in models or just in datasets).

One user-facing change is the order in which the tags are displayed. I'm happy to revert to previous impl if that's wanted.

Some TODOs/questions

  • Add docstrings
  • For PipelineType, we can instead define const ALL_PIPELINE_TYPES = [ "text-classification", ... ] as const; and then const PIPELINES: Record<PipelineType, PipelineData> = { ... }, I left this one as it worked fine and you just have to define the task in a single place. Maybe the other approach was preferred
  • Fix color of icons
  • Remove PIPELINE_TAGS_DISPLAY_ORDER
  • Change imports to don't import type for data
  • Remove []s since they are optional

Some screenshots showing unaffected changes

Screenshot from 2022-03-31 14-59-14
Screenshot from 2022-03-31 14-59-22

Copy link
Member

@julien-c julien-c left a comment

Choose a reason for hiding this comment

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

left a few first comments!

@osanseviero osanseviero mentioned this pull request Mar 31, 2022
5 tasks
@osanseviero osanseviero changed the title [WIP] New task structure New task structure Mar 31, 2022
@osanseviero osanseviero marked this pull request as ready for review March 31, 2022 13:07
@julien-c
Copy link
Member

julien-c commented Apr 1, 2022

@Pierrci @coyotte508 when you have some time (not urgent), can you take a look at the specific question of:

whether we want to type things like in this current PR, or do const ALL_PIPELINE_TYPES = [ "text-classification", ... ] as const; and then const PIPELINES: Record<PipelineType, PipelineData> = { ... }, (which has the advantage of typing the PipelineData subobjects, but the drawback that you need to list each pipeline type twice?

@coyotte508
Copy link
Member

@Pierrci @coyotte508 when you have some time (not urgent), can you take a look at the specific question of:

whether we want to type things like in this current PR, or do const ALL_PIPELINE_TYPES = [ "text-classification", ... ] as const; and then const PIPELINES: Record<PipelineType, PipelineData> = { ... }, (which has the advantage of typing the PipelineData subobjects, but the drawback that you need to list each pipeline type twice?

How it's done here is perfectly fine for me.

But we can have the best of both worlds if you want :)

@julien-c
Copy link
Member

julien-c commented Apr 8, 2022

reminder to self to read @coyotte508's comment on Monday

@julien-c
Copy link
Member

julien-c commented Apr 12, 2022

i have rebased this branch on main locally @osanseviero, i can either push --force here, or push to a new branch (or not do anything 😅) let me know if it's helpful

(as new tasks were added in the meantime)

@julien-c
Copy link
Member

PS/ @coyotte508's typing solution works and is very cool 👍

@osanseviero
Copy link
Contributor Author

Feel free to push to this branch!

Copy link
Member

@julien-c julien-c left a comment

Choose a reason for hiding this comment

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

modulo re-adding a PIPELINE_TAGS_DISPLAY_ORDER (that we can change to feature more non-NLP stuff) this LGTM! 🚢

Copy link
Member

@julien-c julien-c left a comment

Choose a reason for hiding this comment

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

ok this is ready to merge i think!

@osanseviero
Copy link
Contributor Author

Should we wait until moon landing PR is ready before merging?

@julien-c
Copy link
Member

no i think we can merge this and I'll rebase/finish the moon-landing PR this afternoon 👍

@osanseviero osanseviero merged commit db87a77 into main Apr 12, 2022
@osanseviero osanseviero deleted the new-tasks branch April 12, 2022 12:05
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.

4 participants