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.

Sign up for GitHub

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

Jump to bottom

tensorflow: add tf.train.CheckpointOptions and other tf.train members. #11327

Merged
merged 22 commits into from
Feb 1, 2024
Merged

tensorflow: add tf.train.CheckpointOptions and other tf.train members. #11327

merged 22 commits into from
Feb 1, 2024

Conversation

hoel-bagard
Copy link
Contributor

This adds tf.train.CheckpointOptions and the other tf.train members from here.

@hmc-cs-mdrissi If you have time to review it.

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@hoel-bagard hoel-bagard force-pushed the hoel/add_tf_CheckpointOptions branch from 4d5f801 to 6fed7b9 Compare January 27, 2024 13:33
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@hoel-bagard hoel-bagard force-pushed the hoel/add_tf_CheckpointOptions branch from 002b143 to 789b115 Compare January 27, 2024 13:52
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@hoel-bagard hoel-bagard force-pushed the hoel/add_tf_CheckpointOptions branch from c7ad7f2 to a477d9d Compare January 27, 2024 15:07
@hoel-bagard hoel-bagard force-pushed the hoel/add_tf_CheckpointOptions branch from a477d9d to 0f7548c Compare January 27, 2024 15:07
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@hoel-bagard
Copy link
Contributor Author

hoel-bagard commented Jan 27, 2024 •
edited
Loading

@hmc-cs-mdrissi I don't know how to fix the metaclass issue that causes the CI to fail, since google._upb._message.MessageMeta does not seem to exist at runtime. Do you know how this should be handled ?

@hmc-cs-mdrissi
Copy link
Contributor

If you re-export protobuf types from mypy-protobuf generated files that should fix your stubtest metaclass issues.

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
hoel-bagard added a commit to hoel-bagard/typeshed that referenced this pull request Jan 28, 2024
@hoel-bagard
fix: disable stubtest errors for protobuf types.
a2b8165 
See: python#11327 (comment)
@hoel-bagard hoel-bagard force-pushed the hoel/add_tf_CheckpointOptions branch from 8ccd9f4 to a2b8165 Compare January 28, 2024 08:20
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
hoel-bagard added a commit to hoel-bagard/typeshed that referenced this pull request Jan 28, 2024
@hoel-bagard
fix: disable stubtest errors for protobuf types.
d29e821 
See: python#11327 (comment)
@hoel-bagard hoel-bagard force-pushed the hoel/add_tf_CheckpointOptions branch from a2b8165 to d29e821 Compare January 28, 2024 08:27
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@hoel-bagard hoel-bagard force-pushed the hoel/add_tf_CheckpointOptions branch from d29e821 to 794e769 Compare January 28, 2024 08:42
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@hoel-bagard hoel-bagard marked this pull request as ready for review January 28, 2024 08:57
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
hmc-cs-mdrissi
hmc-cs-mdrissi reviewed Jan 31, 2024
View reviewed changes
stubs/tensorflow/tensorflow/train/experimental.pyi Outdated
from _typeshed import Incomplete

# @dataclasses.dataclass(frozen=True)
# class ShardableTensor:
Copy link
Contributor

Choose a reason for hiding this comment

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

For this one I am mainly waiting on either commented out stubs to be finished and not comment or from them to be removed and left for future pr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This class was added to the tensorflow repo last month, so it's not in the doc yet (nor in the latest runtime version). I realized this afterwards and left them comment out to be able to easily add them in the future (I'm assuming the classes will make their way into the next version of tensorflow).
Should I add a comment explaining this, or is it better to remove them for now and add them in a future PR once they have been added to a released version of tensorflow ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend removing them. After next tensorflow release they can be added then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 080b155

@hoel-bagard hoel-bagard force-pushed the hoel/add_tf_CheckpointOptions branch from e8541b9 to 825e16c Compare January 31, 2024 23:31
@hoel-bagard hoel-bagard force-pushed the hoel/add_tf_CheckpointOptions branch from 825e16c to 5532157 Compare January 31, 2024 23:36
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
JelleZijlstra
JelleZijlstra reviewed Feb 1, 2024
View reviewed changes
stubs/tensorflow/tensorflow/train/__init__.pyi
class CheckpointOptions:
experimental_io_device: None | str
experimental_enable_async_checkpoint: bool
# experimental_write_callbacks: None | list[Callable[[str], Any] | Callable[[], Any]]
Copy link
Member

Choose a reason for hiding this comment

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

Are these also due to be added in a future release? I'm OK with leaving them commented out for now, but please put a comment somewhere in the stub about it, so we don't forget why there is this commented out code here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, looking at the 2.12 doc they are not present.
One of them is already in 2.15, two are not present in a release yet but are present in the tensorflow code.

I've removed the ones not present in 2.15, and added comments for the rest in 2ad4be9.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 1, 2024

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@JelleZijlstra JelleZijlstra merged commit 587e75f into python:main Feb 1, 2024
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra left review comments

@hmc-cs-mdrissi hmc-cs-mdrissi hmc-cs-mdrissi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hoel-bagard @hmc-cs-mdrissi @JelleZijlstra