DM-38614: more fixes for storage class conversion support #316
Conversation
TallJimbo
force-pushed
the
tickets/DM-38614
branch
from
23cfa10
to
d76dc2a
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #316 +/- ##
==========================================
- Coverage 80.67% 80.56% -0.12%
==========================================
Files 57 57
Lines 6380 6385 +5
Branches 1303 1304 +1
==========================================
- Hits 5147 5144 -3
- Misses 985 993 +8
Partials 248 248
... and 4 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
TallJimbo
force-pushed
the
tickets/DM-38614
branch
from
06a3a3c
to
d6cbeb1
Compare
If the storage classes are bidirectionally convertible, we just emit a debug message, since this shouldn't cause any trouble. If they're only one-way convertible, it's now an info message rather than a warning, because it's probably fine and the only problem is that the code doing the logging is embarrassed that it can't really tell.
TallJimbo
force-pushed
the
tickets/DM-38614
branch
from
d6cbeb1
to
c4b68be
Compare
| existing.storageClass_name, | ||
| ) | ||
| _dict[datasetType] = combined[existing] | ||
| elif convertible_to_existing or convertible_from_existing: |
There was a problem hiding this comment.
The code in the graph builder tries to work out whether this is solely an output or solely an input or is an intermediate and so doesn't always require that both directions work.
There was a problem hiding this comment.
Are you saying I should drop this message entirely because by the time we get here we've always checked things more thoroughly? I know we've got various checks in various places, but wasn't sure about the flow, so here I just wanted to preserve the original message while making it less scary.
There was a problem hiding this comment.
I think I was mainly commenting on the comment saying that we can't tell whether this is an error or not. I'm not sure where in the graph builder this is occurring so it might be that the test that compare outputs to intermediates to inputs happen later. I do remember trying to be careful about only complaining (and failing to build the graph) if you try to use something as an intermediate that won't convert in both directions (it's possible that is buggy of course).
There was a problem hiding this comment.
Ah, ok, all I meant with the comment was that this code right here doesn't have the information it needs to make that determination. Other parts of the graph builder definitely can.
Checklist
doc/changes