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.

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

Fix xfail in test_linea_api.py #667

Merged

Conversation

mingjerli
Copy link
Contributor

Description

Two reasons causing these two xfail test cases.

  • When we called res.values['x']. I think it is actually asking the tracer to return the last value of x but not the last version of x in the artifact store.
  • ExecuteFixture actually executes the code twice in the test(i don't know why). I think it should be once. This is why you see cannot delete version 1 error.

Fixes # (issue)

Two xfail in test_linea_api.py

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Copy link
Contributor

@yifanwu yifanwu left a comment

Choose a reason for hiding this comment

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

Sure this patch is fine for now.

@@ -138,17 +137,16 @@ def test_delete_artifact_version(execute):
catalog = lineapy.catalog()
versions = [x._version for x in catalog.artifacts if x.name=='x']
num_versions = len(versions)

x_retrieve = lineapy.get('x').get_value()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine, but we can also add accessing artifacts in the TracerContext (which the fixture is using).

It already is accessing artifacts in the following code.

    def slice(self, name: str) -> str:
        artifact = self.db.get_artifact_by_name(name)
        return str(get_program_slice(self.graph, [artifact.node_id]))


# We want to Delete version 1, but the code is executed twice in testing, causing no version 1 to be deleted in second execution
Copy link
Contributor

Choose a reason for hiding this comment

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

We should figure out if this needs to be fixed.

@yifanwu yifanwu merged commit ed0f41f into LIN-425-add-artifact-delete-functionality Jun 3, 2022
@yifanwu yifanwu deleted the LIN-425-test-xfail-patch branch June 3, 2022 21:16
yifanwu pushed a commit that referenced this pull request Jun 5, 2022
* Add delete function to API

Remove an artifact from artifact store as well as
removing value from both database and pickle store if
no other artifacts refer to it.

* Remove ArtifactDeleteException custom exception

A KeyError can be raised instead when the pickle file is not found at
the expected location.

* Write end to end tests for artifact delete

There might be a bug with artifact versions so the tests with hardcoded
versions are xfailed.

* Fix isort

* Add all option to delete API call

Allow user to pass in all as a string into the version parameter
of delete to delete all versions of an artifact.

* Fixing pytorch dependencies for integration tests (#661)

* Fixing pytorch dependencies for integration tests

* Fixing comments for pytorch integration tests

* Fix xfail in test_linea_api.py (#667)

* Fix merged to main branch issue

Co-authored-by: Aayan Kumar <aayan.kumar123@gmail.com>
Co-authored-by: Mingjer Lee <mingjerli@gmail.com>
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.

2 participants