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

remove some warnings #404

Merged
merged 1 commit into from
Sep 10, 2024
Merged

Conversation

EdwardLi-coder
Copy link
Contributor

@EdwardLi-coder EdwardLi-coder commented Sep 7, 2024

When I run the tests, I often encounter warnings related to library dependencies. In my PR, I've ignored warnings coming from third-party libs. And if they come from our own code, I'm rewriting it.
These warnings are need to rewrite code.

tests/unit/lib/test_text.py: 1 warning
  /Users/lifei/PycharmProjects/datachain/.nox/tests-3-12/lib/python3.12/site-packages/datachain/lib/text.py:36: UserWarning: To copy construct from a tensor, it is recommended to use sourceTensor.clone().detach() or sourceTensor.clone().detach().requires_grad_(True), rather than torch.tensor(sourceTensor).
    tokens = torch.tensor(tokens)

tests/unit/lib/test_clip.py::test_similarity_scores_hf
tests/unit/lib/test_image.py::test_convert_image_hf
  /Users/lifei/PycharmProjects/datachain/.nox/tests-3-12/lib/python3.12/site-packages/datachain/lib/image.py:37: UserWarning: To copy construct from a tensor, it is recommended to use sourceTensor.clone().detach() or sourceTensor.clone().detach().requires_grad_(True), rather than torch.tensor(sourceTensor).
    img = torch.tensor(img.pixel_values[0])  # type: ignore[assignment,attr-defined]

tests/unit/lib/test_clip.py::test_similarity_scores_hf
  /Users/lifei/PycharmProjects/datachain/.nox/tests-3-12/lib/python3.12/site-packages/datachain/lib/clip.py:21: UserWarning: To copy construct from a tensor, it is recommended to use sourceTensor.clone().detach() or sourceTensor.clone().detach().requires_grad_(True), rather than torch.tensor(sourceTensor).
    return lambda x: method(torch.tensor(x))

Copy link

codecov bot commented Sep 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.31%. Comparing base (ca36654) to head (3943e4f).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #404   +/-   ##
=======================================
  Coverage   87.31%   87.31%           
=======================================
  Files          92       92           
  Lines        9982     9982           
  Branches     2041     2041           
=======================================
  Hits         8716     8716           
  Misses        911      911           
  Partials      355      355           
Flag Coverage Δ
datachain 87.26% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@shcheklein
Copy link
Member

can we ignore warning that come from third-party libs (e.g. some lib that we depend on is using torch in a deprecated way) and fix warnings on our end by rewriting the code, wdyt?

@EdwardLi-coder
Copy link
Contributor Author

EdwardLi-coder commented Sep 8, 2024

can we ignore warning that come from third-party libs (e.g. some lib that we depend on is using torch in a deprecated way) and fix warnings on our end by rewriting the code, wdyt?

I think I've already done this.
In my PR, I've ignored warnings coming from third-party libs. And if they come from our own code, I'm rewriting it.

@shcheklein
Copy link
Member

In my PR, I've ignored warnings coming from third-party libs. And if they come from our own code, I'm rewriting it.

will it ignore new warnings (let's say someone adds it by mistake tomorrow) in our code? just to double check ...

@EdwardLi-coder
Copy link
Contributor Author

In my PR, I've ignored warnings coming from third-party libs. And if they come from our own code, I'm rewriting it.

will it ignore new warnings (let's say someone adds it by mistake tomorrow) in our code? just to double check ...

These ignore settings are specific to warnings from certain third-party libraries. They will not affect new warnings in our own code. Each ignore has a specific module path, so even if someone accidentally adds new warnings in our code in the future, these settings will not ignore them. New warnings in our code will still be captured and displayed.
However, to ensure we don't miss important warnings, I suggest we periodically review these ignore settings and re-evaluate their necessity after updates to the third-party libraries.

@EdwardLi-coder EdwardLi-coder force-pushed the remove_some_warning branch 2 times, most recently from 7370877 to 38488d5 Compare September 9, 2024 16:25
@@ -141,6 +141,10 @@ filterwarnings = [
"error::pytest_mock.PytestMockWarning",
"error::pytest.PytestCollectionWarning",
"error::sqlalchemy.exc.SADeprecationWarning",
"ignore::DeprecationWarning:timm.*",
"ignore::DeprecationWarning:botocore.auth",
"ignore::DeprecationWarning:datasets.utils._dill",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think maybe only ignore DeprecationWarning is good.

@@ -18,7 +18,7 @@ def _get_encoder(model: Any, type: Literal["image", "text"]) -> Callable:
hasattr(model, method_name) and inspect.ismethod(getattr(model, method_name))
):
method = getattr(model, method_name)
return lambda x: method(torch.tensor(x))
return lambda x: method(torch.as_tensor(x).clone().detach())
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 is the warning:

tests/unit/lib/test_clip.py::test_similarity_scores_hf
  /Users/lifei/PycharmProjects/datachain/.nox/tests-3-12/lib/python3.12/site-packages/datachain/lib/clip.py:21: UserWarning: To copy construct from a tensor, it is recommended to use sourceTensor.clone().detach() or sourceTensor.clone().detach().requires_grad_(True), rather than torch.tensor(sourceTensor).
    return lambda x: method(torch.tensor(x))

Update Reason:
We use torch.as_tensor() to create tensors, which avoids copying data whenever possible. We then call clone().detach() to create a new tensor that is detached from the original data and will not affect the computation graph of the original data.

@@ -33,7 +34,9 @@ def fake_dataset(catalog, fake_image_dir):


def test_pytorch_dataset(fake_dataset):
transform = v2.Compose([v2.ToTensor(), v2.Resize((64, 64))])
transform = v2.Compose(
Copy link
Contributor Author

@EdwardLi-coder EdwardLi-coder Sep 9, 2024

Choose a reason for hiding this comment

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

This is the warning:

  /Users/lifei/PycharmProjects/datachain/.nox/tests-3-12/lib/python3.12/site-packages/torchvision/transforms/v2/_deprecated.py:42: UserWarning: The transform `ToTensor()` is deprecated and will be removed in a future release. Instead, please use `v2.Compose([v2.ToImage(), v2.ToDtype(torch.float32, scale=True)])`.Output is equivalent up to float precision.
    warnings.warn(

update reason:
Compatibility: ToTensor() has been deprecated and may be removed in future versions. Using the new methods ensures your code will continue to work in future versions.

Functional equivalence: The new method combination (v2.ToImage() and v2.ToDtype()) provides the same functionality as ToTensor(), but with more flexibility and clarity.

Precision: The new methods provide output equivalent to ToTensor() up to float precision.

@EdwardLi-coder
Copy link
Contributor Author

@shcheklein I only remain ignore the DeprecationWarning. I have provided some explanations for my modifications. If you have any other suggestions, I would be happy to make further changes.

Copy link
Contributor

@dreadatour dreadatour 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! 👍

@shcheklein
Copy link
Member

thanks @EdwardLi-coder !

@shcheklein shcheklein merged commit 424b05b into iterative:main Sep 10, 2024
32 checks passed
@EdwardLi-coder EdwardLi-coder deleted the remove_some_warning branch September 10, 2024 23:19
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