[ENH] V1 -> V2 Migration - Flows (module)#1609
[ENH] V1 -> V2 Migration - Flows (module)#1609Omswastik-11 wants to merge 306 commits intoopenml:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1609 +/- ##
==========================================
- Coverage 54.67% 54.57% -0.11%
==========================================
Files 63 63
Lines 5108 5153 +45
==========================================
+ Hits 2793 2812 +19
- Misses 2315 2341 +26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
614411f to
36184e5
Compare
|
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
There was a problem hiding this comment.
Nice overall, few changes needed. I'll look at the tests later when the implementation is final.
Signed-off-by: Omswastik-11 <omswastikpanda11@gmail.com>
…into flow-migration-stacked # Conflicts: # openml/_api/resources/__init__.py # openml/_api/resources/base/__init__.py # openml/_api/runtime/core.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
openml/flows/functions.py:67
- get_flow’s docstring still describes the old per-flow XML cache (writing to cache_directory/flows/{flow_id}/flow.xml and raising OpenMLCacheException). The implementation now delegates to openml._backend.flow.get and uses HTTPClient caching, so the documented side effects/raised exceptions are no longer accurate; please update the docstring to match current behavior (and describe the new cache location/semantics if relevant).
"""Fetch an OpenMLFlow by its server-assigned ID.
Queries the OpenML REST API for the flow metadata and returns an
:class:`OpenMLFlow` instance. If the flow is already cached locally,
the cached copy is returned. Optionally the flow can be re-instantiated
into a concrete model instance using the registered extension.
Parameters
----------
flow_id : int
The OpenML flow id.
reinstantiate : bool, optional (default=False)
If True, convert the flow description into a concrete model instance
using the flow's extension (e.g., sklearn). If conversion fails and
``strict_version`` is True, an exception will be raised.
strict_version : bool, optional (default=True)
When ``reinstantiate`` is True, whether to enforce exact version
requirements for the extension/model. If False, a new flow may
be returned when versions differ.
ignore_cache : bool, default=False
Whether to ignore the cache. If ``true`` this will download and overwrite the flow xml
even if the requested flow is already cached.
Returns
-------
OpenMLFlow
The flow object with metadata; ``model`` may be populated when
``reinstantiate=True``.
Raises
------
OpenMLCacheException
When cached flow files are corrupted or cannot be read.
OpenMLServerException
When the REST API call fails.
Side Effects
------------
- Writes to ``openml.config.cache_directory/flows/{flow_id}/flow.xml``
when the flow is downloaded from the server.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
openml/flows/functions.py:59
- The
get_flowdocstring still describes the legacy per-flow XML cache (openml.config.cache_directory/flows/{flow_id}/flow.xml) and listsOpenMLCacheException, butget_flownow delegates toopenml._backend.flow.get(...)which uses the HTTP client cache instead. Please update the docstring (side effects + raises section) to match the new caching/error behavior.
Queries the OpenML REST API for the flow metadata and returns an
:class:`OpenMLFlow` instance. If the flow is already cached locally,
the cached copy is returned. Optionally the flow can be re-instantiated
into a concrete model instance using the registered extension.
Parameters
----------
flow_id : int
The OpenML flow id.
reinstantiate : bool, optional (default=False)
If True, convert the flow description into a concrete model instance
using the flow's extension (e.g., sklearn). If conversion fails and
``strict_version`` is True, an exception will be raised.
strict_version : bool, optional (default=True)
When ``reinstantiate`` is True, whether to enforce exact version
requirements for the extension/model. If False, a new flow may
be returned when versions differ.
Returns
-------
OpenMLFlow
The flow object with metadata; ``model`` may be populated when
``reinstantiate=True``.
Raises
------
OpenMLCacheException
When cached flow files are corrupted or cannot be read.
OpenMLServerException
When the REST API call fails.
Side Effects
------------
- Writes to ``openml.config.cache_directory/flows/{flow_id}/flow.xml``
when the flow is downloaded from the server.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Hi @geetu040 . As per discussion in final mentoring meeting isn't it ready to merge ? |
geetu040
left a comment
There was a problem hiding this comment.
Hi @geetu040 . As per discussion in final mentoring meeting isn't it ready to merge ?
I think this should be ready to merge once the remaining review comments are addressed:
Most of these are related to the tests. Other than that, the implementation looks good to me and should be ready for merge once those points are resolved.
Co-authored-by: Armaghan Shakir <raoarmaghanshakir040@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
openml/flows/functions.py:60
get_flow's docstring still references the removed flows-specific filesystem cache (e.g..../flows/{flow_id}/flow.xml) andOpenMLCacheException, but the implementation now delegates toopenml._backend.flow.get()(HTTP client cache) and no longer raises that exception. Please update the docstring (Raises/Side Effects/Notes) to reflect the new caching behavior and actual exceptions.
Queries the OpenML REST API for the flow metadata and returns an
:class:`OpenMLFlow` instance. If the flow is already cached locally,
the cached copy is returned. Optionally the flow can be re-instantiated
into a concrete model instance using the registered extension.
Parameters
----------
flow_id : int
The OpenML flow id.
reinstantiate : bool, optional (default=False)
If True, convert the flow description into a concrete model instance
using the flow's extension (e.g., sklearn). If conversion fails and
``strict_version`` is True, an exception will be raised.
strict_version : bool, optional (default=True)
When ``reinstantiate`` is True, whether to enforce exact version
requirements for the extension/model. If False, a new flow may
be returned when versions differ.
Returns
-------
OpenMLFlow
The flow object with metadata; ``model`` may be populated when
``reinstantiate=True``.
Raises
------
OpenMLCacheException
When cached flow files are corrupted or cannot be read.
OpenMLServerException
When the REST API call fails.
Side Effects
------------
- Writes to ``openml.config.cache_directory/flows/{flow_id}/flow.xml``
when the flow is downloaded from the server.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Omswastik-11 <omswastikpanda11@gmail.com>
…-11/openml-python into flow-migration-stacked
|
I think Github is having some issues . The diff of this PR looks outdated |
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 .