Skip to content

Collection of nitpicks #12

Closed
Closed

Description

@LysandreJik

See below for a small collection of nitpicks; I suppose those will be adressed before the first release, but wanted to write them down somewhere:

  • Documentation seems to be in a different format so will need to be refactored to work with the doc-builder
  • The logging class still has mentions to 🤗 Transformers
  • You have some tqdm instances in schedulers, but they're not using the utility defined in logging in order to toggle bars on and off
  • Some copyrights have wrong attribution (e.g., configuration_utils which mentions NVIDIA)
  • In some places you define a global variable for configuratio name (e.g., SCHEDULER_CONFIG_NAME = "scheduler_config.json"), in other places you define them as a string (config_name = "model_index.json", in DiffusionPipeline)
  • We have some conventions in transformers that IMO make sense for this library as well, wondering if it is an oversight or a decision to do differently. Those that I can see are: single letter variables, message-less asserts
  • The register_modules doesn't seem to play well with code completion tools
  • The CLIP model is entirely copy/pasted from transformers, is the goal to upstream the change at some point or to keep it separate in that repo?

Activity

  1. patrickvonplaten commented on Jun 15, 2022

    @patrickvonplaten
    Contributor

    All very good points!

    • Doc: Yes we need to change that our style
    • CLIP: idea is to merge it upstream into transformers
    • other points: All yes, we need to adapt this
  2. vvvm23 commented on Jun 16, 2022

    @vvvm23
    Contributor

    Made a small pull request to fix the logging class issues.
    Not a huge contribution, just wanted to get my feet wet with contributing 🙂

    It does change the environmental variables too, but I think this shouldn't have any effects elsewhere - I checked for other occurrences of the variables using ripgrep

  3. patrickvonplaten commented on Jun 16, 2022

    @patrickvonplaten
    Contributor

    Super nice of you @vvvm23 - thanks a lot :-)

    We want to clean up the code and add appropriate licensing / links to original code early next week and we'll soon release a list of todo's for contributions as well if you're interested in contributing more in the coming weeks

  4. vvvm23 commented on Jun 16, 2022

    @vvvm23
    Contributor

    Hi yes, would be interested, feel free to tag me when that happens~ I don't have much experience with continuous diffusion and contributing to big projects, but fairly good at pytorch in general.

  5. patrickvonplaten commented on Jul 21, 2022

    @patrickvonplaten
    Contributor

    Think we applied most of the nitpicks!

  6. added a commit that references this issue on Sep 18, 2023
  7. added a commit that references this issue on Oct 30, 2025
  8. added a commit that references this issue on Aug 11, 2026
  9. added a commit that references this issue on Oct 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions