Skip to content

Conversation

pbeukema
Copy link
Collaborator

@pbeukema pbeukema commented Oct 8, 2024

  • Add api entry point to Dockerfile
  • integration tests built CI not a dummy for now (add github workflows tests in CI in different PR)
  • ...

@pbeukema pbeukema changed the title Pbeukema/fastapi pbeukema/fastapi Oct 8, 2024
@pbeukema pbeukema marked this pull request as draft October 8, 2024 18:50
@yawenzzzz yawenzzzz changed the title pbeukema/fastapi [WIP] pbeukema/fastapi Oct 15, 2024
@yawenzzzz
Copy link
Collaborator

After force push, the log history is cleaned up.

@Hgherzog
Copy link
Collaborator

  • integration tests built CI not a dummy for now (add github workflows tests in CI in different PR)

@pbeukema I am doing github actions in a seperate PR

@yawenzzzz yawenzzzz marked this pull request as ready for review October 17, 2024 22:58
@yawenzzzz yawenzzzz changed the title [WIP] pbeukema/fastapi pbeukema/fastapi Oct 18, 2024
@yawenzzzz yawenzzzz requested a review from favyen2 October 18, 2024 01:06
@yawenzzzz
Copy link
Collaborator

@pbeukema I couldn’t add you as a reviewer, but could you take a look at this PR?​

@yawenzzzz yawenzzzz changed the title pbeukema/fastapi FastAPI for Landsat vessel detection Oct 18, 2024
@yawenzzzz
Copy link
Collaborator

@mikerjacobi Here's the fastapi for landsat.

@favyen2
Copy link
Collaborator

favyen2 commented Oct 21, 2024

I added a test and it looks like everything is working well but originally we were thinking the json_path would be optional since the JSON is also returned in the response body, but right now it got changed to be required in class LandsatRequest (needs to be str | None = None to be optional). Same for scratch_path and crop_path but those are less important to be optional.

image_files: dict[str, str] | None = None
crop_path: str
scratch_path: str
json_path: str
Copy link
Collaborator

Choose a reason for hiding this comment

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

json_path should be optional since Skylight can read the JSON from the response body.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, made json_path optional now.


scene_id: str | None = None
image_files: dict[str, str] | None = None
crop_path: str
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be nice for crop_path to be optional as well. If you don't make it optional then in the test I added, crop_path should be changed to be a subdirectory of the scratch_path, otherwise it writes the crops in the current directory.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed. Also leave it as None in the test, so that we can query API just with a scene_id.

scene_id: str | None = None
image_files: dict[str, str] | None = None
crop_path: str
scratch_path: str
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think scratch_path is fine to be required.

Copy link
Collaborator

@favyen2 favyen2 left a comment

Choose a reason for hiding this comment

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

@yawenzzzz
Copy link
Collaborator

I added a test and it looks like everything is working well but originally we were thinking the json_path would be optional since the JSON is also returned in the response body, but right now it got changed to be required in class LandsatRequest (needs to be str | None = None to be optional). Same for scratch_path and crop_path but those are less important to be optional.

OK, I made all three arguments Optional for api, we may just need the returned json response. And if specified, they can be saved as well.

Copy link
Collaborator

@favyen2 favyen2 left a comment

Choose a reason for hiding this comment

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

The test was failing for me with two issues:

  • tmp_dir.cleanup only exists when doing tmp_dir = tempfile.TemporaryDirectory(), but it was using with tempfile.TemporaryDirectory() as tmp_dir, in the latter case it is just a string while in the former it is object where the directory name (string) can be accessed with tmp_dir.name.
  • b8_fname/rgb_fname were used but not defined if crop_path is not passed.

It seems to be working now. I excluded it from test because it needs GPU, so I put it in tests/integration_slow/.

@yawenzzzz
Copy link
Collaborator

yawenzzzz commented Oct 22, 2024

The test was failing for me with two issues:

  • tmp_dir.cleanup only exists when doing tmp_dir = tempfile.TemporaryDirectory(), but it was using with tempfile.TemporaryDirectory() as tmp_dir, in the latter case it is just a string while in the former it is object where the directory name (string) can be accessed with tmp_dir.name.
  • b8_fname/rgb_fname were used but not defined if crop_path is not passed.

It seems to be working now. I excluded it from test because it needs GPU, so I put it in tests/integration_slow/.

Thanks for the fix! I still got the following error, and it seems the prediction pipeline took very long (> 20 minutes) to run on the test scene. The materializing ran fast, but the preparing windows for classifier took long to complete (more than 1K windows here).

Loaded model weights from the checkpoint at gs://rslearn-eai/projects/rslearn-landsat-recheck/phase123_20240919_01_copy/checkpoints/epoch=27-step=864.ckpt
Predicting: |                                 | 0/? [00:00<?, ?it/s]ERROR:api_main:Unexpected error during prediction pipeline: received 0 items of ancdata

@favyen2
Copy link
Collaborator

favyen2 commented Oct 22, 2024

Could it be not enough shared memory?

@yawenzzzz
Copy link
Collaborator

The test was failing for me with two issues:

  • tmp_dir.cleanup only exists when doing tmp_dir = tempfile.TemporaryDirectory(), but it was using with tempfile.TemporaryDirectory() as tmp_dir, in the latter case it is just a string while in the former it is object where the directory name (string) can be accessed with tmp_dir.name.
  • b8_fname/rgb_fname were used but not defined if crop_path is not passed.

It seems to be working now. I excluded it from test because it needs GPU, so I put it in tests/integration_slow/.

Thanks for the fix! I still got the following error, and it seems the prediction pipeline took very long (> 20 minutes) to run on the test scene. The materializing ran fast, but the preparing windows for classifier took long to complete (more than 1K windows here).

Loaded model weights from the checkpoint at gs://rslearn-eai/projects/rslearn-landsat-recheck/phase123_20240919_01_copy/checkpoints/epoch=27-step=864.ckpt
Predicting: |                                 | 0/? [00:00<?, ?it/s]ERROR:api_main:Unexpected error during prediction pipeline: received 0 items of ancdata

After reducing batch_size, the issue resolved.

@yawenzzzz yawenzzzz merged commit bc424fd into master Oct 22, 2024
4 checks passed
yawenzzzz added a commit that referenced this pull request Jul 3, 2025
FastAPI for Landsat vessel detection
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.

5 participants