Skip to content

Move TODO regarding pickling to markdown#9288

Merged
sydney-runkle merged 1 commit intopydantic:mainfrom
lgeiger:pickling-docs
Apr 23, 2024
Merged

Move TODO regarding pickling to markdown#9288
sydney-runkle merged 1 commit intopydantic:mainfrom
lgeiger:pickling-docs

Conversation

@lgeiger
Copy link
Copy Markdown
Contributor

@lgeiger lgeiger commented Apr 19, 2024 •
edited by pydantic-hooky Bot
Loading

Change Summary

As far as I can tell pickling is supported by pydantic, so I think this TODO in the docs can be removed.

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @adriangb

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 19, 2024 •
edited
Loading

CodSpeed Performance Report

Merging #9288 will not alter performance

Comparing lgeiger:pickling-docs (ffcaa5d) with main (82e4664)

Summary

✅ 13 untouched benchmarks

@lgeiger
Copy link
Copy Markdown
Contributor Author

lgeiger commented Apr 19, 2024

please review

adriangb
adriangb reviewed Apr 19, 2024
View reviewed changes
Comment thread docs/concepts/serialization.md
@adriangb
Copy link
Copy Markdown
Member

adriangb commented Apr 19, 2024

We'll have to get the test working to verify that!

@lgeiger
Copy link
Copy Markdown
Contributor Author

lgeiger commented Apr 19, 2024 •
edited
Loading

We'll have to get the test working to verify that!

Sorry about that, I re-enabled the test. At least locally it seems to work as expected. Let's see what CI thinks.

sydney-runkle
sydney-runkle approved these changes Apr 22, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

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

Great, thank you!

@sydney-runkle
Copy link
Copy Markdown
Contributor

sydney-runkle commented Apr 22, 2024

Oh hmm, looks like this is still failing tests:

data = pickle.dumps(m)
E   _pickle.PicklingError: Can't pickle <class '__main__.FooBarModel'>: attribute lookup FooBarModel on __main__ failed

@lgeiger
Copy link
Copy Markdown
Contributor Author

lgeiger commented Apr 22, 2024

Yeah, looks like the failure just happens in the doctests. I moved the comment to a markdown such that it doesn't show up in the public docs which might confuse some users

@lgeiger lgeiger changed the title Remove outdated TODO regarding pickling in docs Move TODO regarding pickling to markdown Apr 22, 2024
@sydney-runkle sydney-runkle added the relnotes-ignore Omit this PR from the release notes. label Apr 23, 2024
@sydney-runkle
Copy link
Copy Markdown
Contributor

sydney-runkle commented Apr 23, 2024

Ah nice. Looks good!

@sydney-runkle sydney-runkle merged commit 2b8efa2 into pydantic:main Apr 23, 2024
@lgeiger lgeiger deleted the pickling-docs branch April 23, 2024 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adriangb adriangb adriangb left review comments

+1 more reviewer

@sydney-runkle sydney-runkle sydney-runkle approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@adriangb adriangb

Labels

ready for review relnotes-ignore Omit this PR from the release notes.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@lgeiger @adriangb @sydney-runkle