Skip to content

Conversation

@Bojun-Feng
Copy link
Contributor

@Bojun-Feng Bojun-Feng commented Oct 8, 2023

What does this PR do?

Fixes #26638

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@Bojun-Feng Bojun-Feng marked this pull request as ready for review October 8, 2023 04:56
@Bojun-Feng
Copy link
Contributor Author

Bojun-Feng commented Oct 8, 2023

Issues Encountered:

System Configuration:

  • Machine: Mac with M1 Chip
  • Environment: New Conda environment using Python 3.9.18

Installation Issue:

  • Command Executed: pip install -e ".[dev]"
  • Error Encountered:
    INFO: pip is looking at multiple versions of transformers[dev] to determine which version is compatible with other requirements. This could take a while.
    ERROR: Could not find a version that satisfies the requirement tensorflow-text<2.15; extra == "dev" (from transformers[dev]) (from versions: none)
    ERROR: No matching distribution found for tensorflow-text<2.15; extra == "dev"
    
  • Solution: Replaced ".[dev]" with ".[quality]" and the module was successfully installed.

Execution Issue:

  • Command Executed: python3 utils/check_docstrings.py --fix_and_overwrite
  • Error Encountered:
    Traceback (most recent call last):
      File ".../transformers/src/transformers/utils/import_utils.py", line 1282, in _get_module
        return importlib.import_module("." + module_name, self.__name__)
      File ".../python3.9/importlib/__init__.py", line 127, in import_module
        return _bootstrap._gcd_import(name[level:], package, level)
      File "<frozen importlib._bootstrap>", line 1030, in _gcd_import
      File "<frozen importlib._bootstrap>", line 1007, in _find_and_load
      File "<frozen importlib._bootstrap>", line 986, in _find_and_load_unlocked
      File "<frozen importlib._bootstrap>", line 680, in _load_unlocked
      File "<frozen importlib._bootstrap_external>", line 850, in exec_module
      File "<frozen importlib._bootstrap>", line 228, in _call_with_frames_removed
      File ".../transformers/src/transformers/models/deta/modeling_deta.py", line 52, in <module>
        from torchvision.ops.boxes import batched_nms
      File ".../site-packages/torchvision/__init__.py", line 6, in <module>
        from torchvision import _meta_registrations, datasets, io, models, ops, transforms, utils
      File ".../site-packages/torchvision/datasets/__init__.py", line 1, in <module>
        from ._optical_flow import FlyingChairs, FlyingThings3D, HD1K, KittiFlow, Sintel
      File ".../site-packages/torchvision/datasets/_optical_flow.py", line 10, in <module>
        from PIL import Image
    ModuleNotFoundError: No module named 'PIL'
    
  • Solution: Used python instead of python3 and the file executed successfully.
  • However, only doc strings in CodeLlamaTokenizerFast were modified. CI tests failed for CodeLlamaTokenizer.

Next Steps:

  • Add CodeLlamaTokenizer back into OBJECTS_TO_IGNORE.
  • See if doc strings of CodeLlamaTokenizerFast are updated correctly and can pass the CI.

@Bojun-Feng Bojun-Feng changed the title [docstring] Fix docstring for CodeLlamaTokenizer, CodeLlamaTokenizerFast [docstring] Fix docstring for CodeLlamaTokenizerFast Oct 8, 2023
@Bojun-Feng
Copy link
Contributor Author

Bojun-Feng commented Oct 8, 2023

Hi @ydshieh,

Doc strings of CodeLlamaTokenizerFast seems to be properly updated, and this PR is ready to merge.

However, the above dependency issues were a bit confusing and prevented me from updating the doc strings of CodeLlamaTokenizer. Am I setting up the dev environment incorrectly? Any information would be much appreciated!

@ydshieh
Copy link
Collaborator

ydshieh commented Oct 9, 2023

cc @ydshieh

@Bojun-Feng
Copy link
Contributor Author

Update on issues encountered: pip install -e ".[dev-torch]" solved the issue for me.

Copy link
Collaborator

@ydshieh ydshieh left a comment

Choose a reason for hiding this comment

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

Very nice, thank you for the contribution 🔥 💯 .

I have one question and needs my colleague to confirm.

@ydshieh
Copy link
Collaborator

ydshieh commented Oct 11, 2023

Regarding environment, I am not using Mac, and it seems tensorflow related stuffs are in the way (tensorflow-text).

Do you have tensorflow and/or tensorflow-text installed? What are their versions?

@Bojun-Feng
Copy link
Contributor Author

Regarding environment, I am not using Mac, and it seems tensorflow related stuffs are in the way (tensorflow-text).

Do you have tensorflow and/or tensorflow-text installed? What are their versions?

I have tensorflow at version 2.14.0. I do not have tensorflow-text.

@ydshieh
Copy link
Collaborator

ydshieh commented Oct 11, 2023

Maybe you can remove the 3 places of tensorflow-text in the file setup.py (do not commit) and try install with .[dev] again, and see if this could resolve issue. But we can move on to merge this PR just with CodeLlamaTokenizerFast. We can leave CodeLlamaTokenizer for another PR.

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

@Bojun-Feng
Copy link
Contributor Author

Maybe you can remove the 3 places of tensorflow-text in the file setup.py (do not commit) and try install with .[dev] again, and see if this could resolve issue. But we can move on to merge this PR just with CodeLlamaTokenizerFast. We can leave CodeLlamaTokenizer for another PR.

Let's just merge it. I can create another PR for CodeLlamaTokenizer.

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

I can open the follow up PR to fix the suffix first issue!

@ArthurZucker
Copy link
Collaborator

Could you resolve the conflicts so that I can approve the pR? 😉

@Bojun-Feng
Copy link
Contributor Author

Could you resolve the conflicts so that I can approve the pR? 😉

I am a bit unsure what you mean by conflict. I have resolved the conversation, if that is what you meant. Please let me know if you would like me to add back the suffix_first parameter.

@ydshieh
Copy link
Collaborator

ydshieh commented Oct 13, 2023

@Bojun-Feng

Check This branch has conflicts that must be resolved below. It has conflict to the main branch as the (long) list of OBJECTS_TO_IGNORE is updated frequently due to the event.

@Bojun-Feng
Copy link
Contributor Author

@Bojun-Feng

Check This branch has conflicts that must be resolved below. It has conflict to the main branch as the (long) list of OBJECTS_TO_IGNORE is updated frequently due to the event.

Conflict resolved, thank you for the clarification!

Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Thanks for fixing 😉

@ArthurZucker ArthurZucker merged commit 5c081e2 into huggingface:main Oct 16, 2023
EduardoPach pushed a commit to EduardoPach/transformers that referenced this pull request Nov 19, 2023
…6666)

* remove from OBJECTS_TO_IGNORE

* run check_docstrings.py

* fill in information

* ignore CodeLlamaTokenizer
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.

[Community Event] Docstring Sprint

4 participants