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

chore: Add py37 warning #720

Merged
merged 3 commits into from
Jul 25, 2023
Merged

chore: Add py37 warning #720

merged 3 commits into from
Jul 25, 2023

Conversation

setu4993
Copy link
Member

@setu4993 setu4993 commented Jul 24, 2023

During the import of dataquality, start warning users that we're going to be dropping Python 3.7 support soon.

@setu4993 setu4993 requested review from a team and dcaustin33 as code owners July 24, 2023 16:02
Copy link
Contributor

@elboy3 elboy3 left a comment

Choose a reason for hiding this comment

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

Why in the Analytics init? What not in dq.init or in the __init__.py file when you import?

@setu4993
Copy link
Member Author

@elboy3 :

Why in the Analytics init? What not in dq.init or in the __init__.py file when you import?

I put it here because that's where we were logging the version to Amplitude for. I personally prefer putting it in __init__.py but wasn't sure if that'd be okay. Can move it there.

@codecov-commenter
Copy link

codecov-commenter commented Jul 24, 2023

Codecov Report

Merging #720 (6e05e74) into main (22a1a40) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #720      +/-   ##
==========================================
- Coverage   89.54%   89.50%   -0.04%     
==========================================
  Files         166      166              
  Lines       13403    13393      -10     
==========================================
- Hits        12002    11988      -14     
- Misses       1401     1405       +4     
Files Changed Coverage Δ
dataquality/integrations/ultralytics.py 85.18% <100.00%> (-0.07%) ⬇️

... and 6 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@elboy3
Copy link
Contributor

elboy3 commented Jul 24, 2023

@setu4993 I think i'd prefer that, since we might remove amplitude in the not so far future as well

@setu4993
Copy link
Member Author

@elboy3 :

@setu4993 I think i'd prefer that, since we might remove amplitude in the not so far future as well

Fair enough, done.

@setu4993 setu4993 merged commit 45415c5 into main Jul 25, 2023
@setu4993 setu4993 deleted the chore/py37-warn branch July 25, 2023 01:32
setu4993 added a commit that referenced this pull request Jul 25, 2023
Turns out the test that was failing in #720 was just being flaky,
because of an upstream change in settings management for `ultralytics`
(ultralytics/ultralytics#3790).

This improves how we handle it.

Tests:
- [x] Local.
- [ ] CI.
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