Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Data edit api #1044

Merged
merged 2 commits into from
Jul 10, 2020
Merged

Data edit api #1044

merged 2 commits into from
Jul 10, 2020

Conversation

sahithyaravi
Copy link
Member

@sahithyaravi sahithyaravi commented May 13, 2020 •
edited
Loading

As discussed, this api can edit some meta-features of the dataset:
List of meta-features can be edited/ updated via data_edit API call,

  1. description
  2. creator
  3. contributor
  4. collection_date
  5. language
  6. citation
  7. original_data_url
  8. paper_url

These meta-features/ data will just need to call create_dataset api and create a new version: (python api)

  1. attributes
  2. data - the data itself
  3. default_target_attribute
  4. ignore_attribute
  5. row_id_attribute

After the data_edit API is ready, I can create a single python API to handle both of these cases.
Based on the arguments (None or otherwise). We can either create a new dataset or call the data_edit API.

How to test this API?

Create a post request:
Request URL: localhost/api/v1/json/data/edit
Request body: Specify the data_id and atleast one of the 7 fields to edit as an XML

image
example data.xml:

<oml:data_edit_parameters xmlns:oml="http://openml.org/openml">
<oml:description> Testing description changes </oml:description>
<oml:creator> sahi </oml:creator>
<oml:contributor> Joaquin Vanschoren</oml:contributor>
<oml:collection_date> 2020 </oml:collection_date>
<oml:language>English</oml:language>
<oml:citation>xxx</oml:citation>
<oml:original_data_url>http://openml.org/d/1</oml:original_data_url>
<oml:paper_url>http://openml.org/d/1</oml:paper_url>
</oml:data_edit_parameters>

@sahithyaravi sahithyaravi changed the base branch from master to develop May 13, 2020 11:51
@sahithyaravi sahithyaravi self-assigned this May 13, 2020
@joaquinvanschoren joaquinvanschoren mentioned this pull request Jun 22, 2020
Open
@sahithyaravi sahithyaravi mentioned this pull request Jul 7, 2020
Closed
janvanrijn
janvanrijn requested changes Jul 7, 2020
View reviewed changes
Copy link
Member

@janvanrijn janvanrijn left a comment

Choose a reason for hiding this comment

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

Code looking good @sahithyaravi1493, I could only find some small things that should be improved.

openml_OS/models/api/v1/Api_data.php
@@ -232,6 +237,66 @@ private function data_list($segs) {
$this->xmlContents('data', $this->version, array('datasets' => $datasets));
}

private function data_edit() {
// Get columns to be update from post data
$data_id = $this->input->post('data_id');
Copy link
Member

Choose a reason for hiding this comment

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

For consistency, it would be great if we would solve this with a XML (and new XSD) as well.

openml_OS/models/api/v1/Api_data.php Outdated

// If data id is not given
if( $data_id == false ) {
$this->returnError( 110, $this->version );
Copy link
Member

Choose a reason for hiding this comment

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

For consistency, the API error codes are setup in such a way that we have a "range" of error codes per error function. It would be good to claim the range 1060 - 1069 for this function, and duplicate all error messages that are now reused.

openml_OS/models/api/v1/Api_data.php Outdated
// where data id
$where_data = 'where did='. $data_id;

$this->Dataset->query('update dataset set '. $update_total . $where_data);
Copy link
Member

Choose a reason for hiding this comment

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

please use Codeigniter official Query for this (it's $this->Dataset->update or something like that)

openml_OS/models/api/v1/Api_data.php Outdated
$where_data = 'where did='. $data_id;

$this->Dataset->query('update dataset set '. $update_total . $where_data);

Copy link
Member

Choose a reason for hiding this comment

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

please also 'catch' the return value of that function (boolean) and handle it if there is an error

openml_OS/models/api/v1/Api_data.php Outdated
$this->Dataset->query('update dataset set '. $update_total . $where_data);

// Return edited dataset, for user to verify changes
$this->xmlContents( 'data-get', $this->version, $dataset );
Copy link
Member

Choose a reason for hiding this comment

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

For consistency (with upload/delete fns), I would suggest to create a 'data-edit' and to only show data id

janvanrijn
janvanrijn approved these changes Jul 10, 2020
View reviewed changes
Copy link
Member

@janvanrijn janvanrijn left a comment

Choose a reason for hiding this comment

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

Thanks for making this PR! It looks great.

@joaquinvanschoren joaquinvanschoren merged commit bcc97b8 into develop Jul 10, 2020
@sahithyaravi sahithyaravi deleted the data_edit_api branch August 5, 2020 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janvanrijn janvanrijn janvanrijn approved these changes

@joaquinvanschoren joaquinvanschoren Awaiting requested review from joaquinvanschoren

Assignees

@sahithyaravi sahithyaravi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sahithyaravi @janvanrijn @joaquinvanschoren