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

Migrate all tests to use Flask #111

Merged
merged 20 commits into from
Apr 27, 2024
Merged

Migrate all tests to use Flask #111

merged 20 commits into from
Apr 27, 2024

Conversation

neshdev
Copy link
Member

@neshdev neshdev commented Apr 26, 2024

  • Tests were failing on mac osx sporadically. Switching to a different web server seems to have solved the issues.
  • Added vscode files for vsode ide development.
  • ColabTestServer, JwtTestServer and HttpTestServer have been migrated to flask.
  • Only minor modifications to the actual test
  • Tried to use "native" functionality in flask for all requests
    b/337248131

@neshdev neshdev requested review from rosbo and mohami2000 April 26, 2024 18:48
Copy link
Contributor

@rosbo rosbo 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 factoring out the stub in separate files to help with future maintainability.

LGTM, just some minor questions / suggestions.

pyproject.toml Outdated
@@ -32,10 +32,21 @@ dependencies = [
[tool.hatch.version]
path = "src/kagglehub/__init__.py"

[project.optional-dependencies]
dev = [
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed yesterday, is this block still needed? Probably best to standardize on always using hatch.

Copy link
Member Author

Choose a reason for hiding this comment

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

sg. Hatch works natively, I will remove this.



@contextmanager
def create_env() -> Generator[Any, Any, Any]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the type Generator[Any, Any, Any]? At line #78, you don't yield anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was the suggested type from mypy. See attached screenshot.

Generators can be used for bidirectional communication. From my understanding, the way the contextmanager works is by using generators to stop execution at the first yield statement. After, the with statement body of the users function completes. Then, the program resumes execution from yield statement.

01 @contextmanager
02 def my_custom_open():
03     try:
04         f = open("a/b/c/d.txt", "wb")
05         yield f
06.    finally:
07         f.close()
08       
09  def foo():
10       with my_custom_open() as f:
11            chunk1 = f.read(1024)
12           chunk2 = f.read(1024)

For ex, in the above example, if the user executes the foo function on line 09, the code execution for my_custom_open is advanced to line 05. After, line 11 and 12 are resumed and completed. Finally, the execution resumes at line 05 to completion.

Screenshot 2024-04-26 at 11 14 42 PM

Copy link
Contributor

@rosbo rosbo Apr 29, 2024

Choose a reason for hiding this comment

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

Sorry, I wasn't clear. I am familiar with how contextmanager works, I setup these test harness methods.

My suggestion was around the exact type for Generator.

You have to specify three types: the yield, send and return type.

Given you yield Nothing, the yield type should be None instead of Any. Same for the send and return typs, we don't send or return values into the generator.

class TestColabCacheModelDownload(BaseTestCase):
@classmethod
def setUpClass(cls): # noqa: ANN102
# TODO: the colab environment has flaky behavior if set in test. It also can't be set in the `tests/__init__.py`
Copy link
Contributor

Choose a reason for hiding this comment

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

What's flaky? Is it the server not responsive or not using the right address? Given it is simply calling a local http server, I wouldn't expect any flakiness.

Copy link
Member Author

Choose a reason for hiding this comment

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

I documented this issue in this b/337257114

@app.route("/api/v1/models/<org_slug>/<model_slug>/create/instance", methods=["POST"])
def model_instance_create_instance(org_slug: str, model_slug: str) -> ResponseReturnValue:
post_data = request.get_json()
# TODO: error here
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this TODO? Same below.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added this while refactoring. Forgot to take this out.

@neshdev neshdev merged commit bd12f97 into main Apr 27, 2024
7 checks passed
@neshdev neshdev deleted the neshdev/flask-all-test branch April 27, 2024 04:23
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