[ENH] V1 -> V2 Migration - Flows (module) #1609
Conversation
Omswastik-11
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1609 +/- ##
==========================================
- Coverage 52.75% 50.21% -2.55%
==========================================
Files 36 47 +11
Lines 4333 4634 +301
==========================================
+ Hits 2286 2327 +41
- Misses 2047 2307 +260 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Omswastik-11 <[email protected]>
Omswastik-11
force-pushed
the
flow-migration-stacked
branch
from
614411f to
36184e5
Compare
for more information, see https://pre-commit.ci
|
Hi @geetu040 !! Can you review the pre-commit failure It was due to merge conflicts more specifically for tasks . Should I change it on my branch ? |
can you try again, sync your branch with mine? It should be fixed now. |
|
I think that due to conflicts it did not synced properly . I have to revert it manually |
geetu040
left a comment
geetu040
left a comment
There was a problem hiding this comment.
Nice overall, few changes needed. I'll look at the tests later when the implementation is final.
| flow_cache_dir = openml.utils._create_cache_directory(FLOWS_CACHE_DIR_NAME) | ||
| directory_content = os.listdir(flow_cache_dir) # noqa: PTH208 | ||
| directory_content.sort() | ||
| directory_content = sorted(p.name for p in flow_cache_dir.iterdir()) |
There was a problem hiding this comment.
can't these pre-commit checks stays away from this PR?
There was a problem hiding this comment.
Should I revert this ?
There was a problem hiding this comment.
yes if it doesn't block anything else, though I don't mind these changes but let's try to keep the PR straight to the point
| return flows["id"].to_list() # type: ignore[no-any-return] | ||
|
|
||
|
|
||
| def __list_flows(api_call: str) -> pd.DataFrame: |
There was a problem hiding this comment.
shouldn't this function __list_flows as well as _list_flows be simply removed now?
There was a problem hiding this comment.
I thought these might be used some where out of that file so I didn't removed them should i remove them ? Very similar implementation will be done on runs module .
There was a problem hiding this comment.
yes remove them, I don't think they are used elsewhere, but please confirm this
4 participants
Fixes #1601
added a
Createmethod inFlowAPIfor publishing flow but not refactored with oldpublish. (Needs discussion on this)Added tests using
fake_methodsso that we can test without localV2server . I have tested theFlowsV2methods (getandexists) anddeleteandlistwere not implemented inV2server so I skipped them .