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

[MAINTENANCE] Most Recent Version of matplotlib breaks ptitprince and seaborn method calls. #4007

Conversation

alexsherstinsky
Copy link
Collaborator

@alexsherstinsky alexsherstinsky commented May 22, 2024

Scope

  • We must pin matplotlib to "<3.9.0", because the latter (released on 5/15/2024), cause API incompatibilities:
-------- generated xml file: /home/runner/work/ludwig/ludwig/pytest.xml --------
=========================== short test summary info ============================
ERROR tests/integration_tests/test_visualization.py - AttributeError: module 'matplotlib.cm' has no attribute 'register_cmap'
ERROR tests/integration_tests/test_visualization_api.py - AttributeError: module 'matplotlib.cm' has no attribute 'register_cmap'
!!!!!!!!!!!!!!!!!!! Interrupted: 2 errors during collection !!!!!!!!!!!!!!!!!!!!
==== 2 skipped, 4[93](https://github.com/ludwig-ai/ludwig/actions/runs/9151376924/job/25157299861?pr=3981#step:7:94)3 deselected, 2 warnings, 2 errors in 357.57s (0:05:57) =====
Error: Process completed with exit code 2.

thus breaking the tests collection. Pinning the version of matplotlib fixes this issue. We will keep an eye on the future matplotlib releases, which might render the pinning of its version unnecessary.

  • We also fix the ViTEncoder to ensure that the transformers.ViTModel returns the output_attentions.

Code Pull Requests

Please provide the following:

  • a clear explanation of what your code does
  • if applicable, a reference to an issue
  • a reproducible test for your PR (code, config and data sample)

Documentation Pull Requests

Note that the documentation HTML files are in docs/ while the Markdown sources are in mkdocs/docs.

If you are proposing a modification to the documentation you should change only the Markdown files.

api.md is automatically generated from the docstrings in the code, so if you want to change something in that file, first modify ludwig/api.py docstring, then run mkdocs/code_docs_autogen.py, which will create mkdocs/docs/api.md .

Copy link

github-actions bot commented May 22, 2024

Unit Test Results

  6 files  ±0    6 suites  ±0   14m 10s ⏱️ - 2m 57s
12 tests ±0    9 ✔️ ±0    3 💤 ±0  0 ±0 
60 runs  ±0  42 ✔️ ±0  18 💤 ±0  0 ±0 

Results for commit b20a68c. ± Comparison against base commit b6df715.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@arnavgarg1 arnavgarg1 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, I wonder why they'd make a hard requirement change of explicitly setting the generation config though

@@ -563,6 +563,8 @@ def save(self, save_path):
# avoid this hack
if self.config_obj.trainer.type != "none":
weights_save_path = os.path.join(save_path, MODEL_WEIGHTS_FILE_NAME)
# We initialize the model's generation configuration; otherwise, we get a validation error.
self.model.generation_config = self.generation
Copy link
Contributor

Choose a reason for hiding this comment

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

Fascinating

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@arnavgarg1 Yes, and they also changed the ViTModel (I filed an issue: huggingface/transformers#30978). Unless you recommend changes, may I please get an approval so that @ethanreidel can continue his work? Thanks!

@alexsherstinsky alexsherstinsky merged commit 7053966 into master May 23, 2024
18 checks passed
@alexsherstinsky alexsherstinsky deleted the maintenance/alexsherstinsky/requirements/restrict_matplotlib_to_less_than_version_3point9_due_to_seaborn_incompatibility-2024_05_22-36 branch May 23, 2024 16:24
skanjila pushed a commit to skanjila/ludwig that referenced this pull request Jun 7, 2024
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.

3 participants