Runtime dependencies check #1670
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1670 +/- ##
==========================================
- Coverage 80.58% 77.38% -3.21%
==========================================
Files 302 304 +2
Lines 15484 15439 -45
==========================================
- Hits 12478 11947 -531
- Misses 3006 3492 +486
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
src/neptune/exceptions.py
Outdated
| framework_name=framework_name, | ||
| **STYLES, | ||
| ) | ||
| ) | ||
|
|
||
|
|
||
| class NeptuneIntegrationNotInstalledException(NeptuneMissingRequirementException): |
There was a problem hiding this comment.
@Raalsky tbh I'm not sure I see a need for a separate exception type for integrations. It makes it all more complicated. How about we just throw a NeptuneMissingRequirementException and always link a docs page for integrations? Also @normandy7, what do you think?
There was a problem hiding this comment.
You mean removing the installation instructions from the exception message? What are the pros/cons with this change (from an end-user perspective)? It would be ideal to have an installation command handy for copy-pasting
There was a problem hiding this comment.
No no, actually it will be just about renaming NeptuneIntegrationNotInstalledException to NeptuneMissingRequirementException and small tweaks in the message (changing 'integration' to just 'package') to make it more generic, as we'll also probably be checking installation of things like pandas which is not an integration per se
There was a problem hiding this comment.
The rest will stay the same - installation instruction and link to the docs. I just don't see a need to create two separate entities which only differ in names and some small message wording details
There was a problem hiding this comment.
Oh I see! Yes, that should be fine.
There was a problem hiding this comment.
I will introduce a simplification shortly and ask you to review, ok? 😃
There was a problem hiding this comment.
Certainly 👍🏻 I'll monitor my inbox
There was a problem hiding this comment.
There's a lot of files, but it's mostly just updating how integrations are imported. The thing to check is probably only src/neptune/exceptions.py :)
Co-authored-by: Sabine <sabine.nyholm@neptune.ai>
| from neptune.exceptions import NeptuneMissingRequirementException | ||
|
|
||
|
|
||
| def is_installed(requirement_name: str) -> bool: |
There was a problem hiding this comment.
Are we sure that this is going to be cached? Maybe can we just use some simple LRU cache directly?
Before submitting checklist