Skip to content

[ENH] V1 -> V2 Migration - Flows (module)#1609

Open
Omswastik-11 wants to merge 318 commits into
openml:mainfrom
Omswastik-11:flow-migration-stacked
Open

[ENH] V1 -> V2 Migration - Flows (module)#1609
Omswastik-11 wants to merge 318 commits into
openml:mainfrom
Omswastik-11:flow-migration-stacked

Conversation

@Omswastik-11
Copy link
Copy Markdown
Contributor

@Omswastik-11 Omswastik-11 commented Jan 8, 2026

Fixes #1601

added a Create method in FlowAPI for publishing flow but not refactored with old publish . (Needs discussion on this)
Added tests using fake_methods so that we can test without local V2 server . I have tested the FlowsV2 methods (get and exists ) and delete and list were not implemented in V2 server so I skipped them .

@Omswastik-11 Omswastik-11 changed the title [ENH] V1 -> V2 Migration [ENH] V1 -> V2 Migration - Flows (module) Jan 8, 2026
@Omswastik-11 Omswastik-11 marked this pull request as ready for review January 8, 2026 15:09
@geetu040 geetu040 mentioned this pull request Jan 9, 2026
18 tasks
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 12, 2026

Codecov Report

❌ Patch coverage is 39.47368% with 69 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.53%. Comparing base (c62bf51) to head (915b4cb).

Files with missing lines Patch % Lines
openml/_api/resources/flow.py 40.21% 55 Missing ⚠️
openml/flows/flow.py 26.66% 11 Missing ⚠️
openml/flows/functions.py 57.14% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1609      +/-   ##
==========================================
- Coverage   54.64%   54.53%   -0.11%     
==========================================
  Files          63       63              
  Lines        5124     5169      +45     
==========================================
+ Hits         2800     2819      +19     
- Misses       2324     2350      +26     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Collaborator

@geetu040 geetu040 left a comment

Choose a reason for hiding this comment

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

Nicely done overall.

Comment thread openml/_api/resources/base.py Outdated
Comment thread openml/_api/resources/base/resources.py Outdated
Comment thread openml/_api/resources/flows.py Outdated
Comment thread openml/_api/resources/flow.py Outdated
Comment thread openml/flows/functions.py Outdated
Comment thread openml/flows/functions.py Outdated
Comment thread openml/flows/functions.py Outdated
Comment thread tests/test_flows/test_flow_functions.py
Comment thread tests/test_flows/test_flow_migration.py Outdated
Comment thread openml/_api/resources/flows.py Outdated
@Omswastik-11 Omswastik-11 force-pushed the flow-migration-stacked branch from 614411f to 36184e5 Compare January 14, 2026 14:56
@Omswastik-11 Omswastik-11 requested a review from geetu040 January 14, 2026 15:10
@Omswastik-11
Copy link
Copy Markdown
Contributor Author

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 ?

@geetu040
Copy link
Copy Markdown
Collaborator

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.

@Omswastik-11
Copy link
Copy Markdown
Contributor Author

I think that due to conflicts it did not synced properly . I have to revert it manually

Copy link
Copy Markdown
Collaborator

@geetu040 geetu040 left a comment

Choose a reason for hiding this comment

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

Nice overall, few changes needed. I'll look at the tests later when the implementation is final.

Comment thread openml/_api/resources/flows.py Outdated
Comment thread openml/flows/functions.py Outdated
Comment thread openml/flows/functions.py Outdated
@Omswastik-11 Omswastik-11 requested a review from geetu040 January 21, 2026 18:25
@Omswastik-11 Omswastik-11 marked this pull request as draft January 27, 2026 07:30
@Omswastik-11 Omswastik-11 marked this pull request as ready for review January 27, 2026 15:13
Copy link
Copy Markdown
Collaborator

@geetu040 geetu040 left a comment

Choose a reason for hiding this comment

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

sdk code look good so far, please take a look at #1575 (comment) and make changes accordingly where needed.
all tests (existing and new) should pass to make sure we are retaining the original functionality of the sdk

Comment thread openml/flows/functions.py Outdated
Comment thread openml/flows/functions.py Outdated
Comment thread tests/test_api/test_flow.py
assert subflow.flow_id == sub_flow_id

@pytest.mark.test_server()
def test_tagging(self):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this test in theory should work as before, if it's passing in main, it should pass here as well.

Copy link
Copy Markdown
Contributor Author

@Omswastik-11 Omswastik-11 May 11, 2026

Choose a reason for hiding this comment

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

Link
What should we do here ?

Copy link
Copy Markdown
Contributor Author

@Omswastik-11 Omswastik-11 May 11, 2026

Choose a reason for hiding this comment

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

The migrated FlowV1API.list() uses HTTP caching (enable_cache=True). After push_tag → list_flows(tag=tag) returns 1 result and caches it. After remove_tag, the next list_flows(tag=tag) returns the stale cached result (1 instead of 0). The old code path didn't cache flow listings.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I see, we shouldn't use cache here. See #1609

geetu040 and others added 2 commits May 11, 2026 16:41
Co-authored-by: Armaghan Shakir <raoarmaghanshakir040@gmail.com>
Copilot AI review requested due to automatic review settings May 11, 2026 12:21
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Comment thread openml/flows/functions.py Outdated
Comment thread tests/test_api/test_flow.py Outdated
Comment thread openml/flows/flow.py
Comment thread openml/flows/flow.py
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings May 11, 2026 12:29
@Omswastik-11 Omswastik-11 requested a review from geetu040 May 11, 2026 12:34
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Comment thread tests/test_api/test_flow.py Outdated
Comment thread openml/flows/flow.py
Copilot AI review requested due to automatic review settings May 11, 2026 13:17
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

@pytest.mark.test_server()
def test_flow_v1_get(flow_v1):
flow = flow_v1.get(flow_id=1)
_validate_flow(flow)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can be ignored, see #1609 (comment)

@pytest.mark.test_server()
def test_flow_v2_get(flow_v2):
flow = flow_v2.get(flow_id=1)
_validate_flow(flow)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can be ignored, see #1609 (comment)

Comment thread tests/test_api/test_flow.py Outdated
Comment thread tests/test_api/test_flow.py Outdated
Comment thread openml/_api/resources/flow.py Outdated
Omswastik-11 and others added 2 commits May 11, 2026 19:47
Co-authored-by: Armaghan Shakir <raoarmaghanshakir040@gmail.com>
Copilot AI review requested due to automatic review settings May 11, 2026 14:18
Co-authored-by: Armaghan Shakir <raoarmaghanshakir040@gmail.com>
@Omswastik-11 Omswastik-11 requested a review from geetu040 May 11, 2026 14:30
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Copy Markdown
Collaborator

@geetu040 geetu040 left a comment

Choose a reason for hiding this comment

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

Nice work @Omswastik-11.

@PGijsbers please review/merge.

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.

[ENH] V1 → V2 API Migration - flows

7 participants