Skip to content

Conversation

@grallewellyn
Copy link
Member

Notable changes:

  • username is no longer needed for maap-py.submitJob
  • I added the note about setting USER because I was confused at first if USER was the owner of the algorithm or the user running this notebook
  • earthengine-api is not already in the pangeo image, should it be added?

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@grallewellyn grallewellyn requested review from smk0033 and wildintellect and removed request for HarshiniGirish January 5, 2026 22:55
@wildintellect
Copy link
Collaborator

Notable changes:

* username is no longer needed for maap-py.submitJob

Good to know.

* I added the note about setting USER because I was confused at first if USER was the owner of the algorithm or the user running this notebook

It should be the person running the notebook. But that raises and interesting issue. I think hard coding the output directory to S3 as an algorithm variable is probably not a good idea. We probably need open a bug on the algorithm repo to fix that.

* earthengine-api is not already in the pangeo image, should it be added?

At this time no, I think we'll keep it as a user add-on, trying to balance default vs kitchen sink. Because of that maybe we need the conda install command in a code block at the top.

@grallewellyn
Copy link
Member Author

Added conda install command at the top

I think hard coding the output directory to S3 as an algorithm variable is probably not a good idea. We probably need open a bug on the algorithm repo to fix that.

How else would specifying the output directory be done?

@wildintellect
Copy link
Collaborator

How else would specifying the output directory be done?

🤔 /output in a DPS job automatically goes to a users private bucket, and the DPS API getresults should provide the path. At least that's the traditional way. There is 1 DPS arg that does allow modifying the output path. I'll need to look more in depth at the algorithm before commenting more -- it might be doing direct S3 writes bypassing DPS file handling. But this is an algorithm change first. The notebook as written is correct given the current algorithm.

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.

3 participants