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

Readme commands are wrong #7

Open
wildintellect opened this issue Nov 22, 2024 · 6 comments
Open

Readme commands are wrong #7

wildintellect opened this issue Nov 22, 2024 · 6 comments
Assignees

Comments

@wildintellect
Copy link

Most of the commands in the Readme don't work at expected unless you add python to the front of them to make sure the correct python is used.

example, no idea why this would trigger an env build

(pangeo) root@workspacexr0kewu7brp8963d:~/sar2-d2# nasa/algorithm/register.py 
ERROR: The SAR2D2_ENV environment variable must be set to the name of the conda environment to run in.

works correct

(pangeo) root@workspacexr0kewu7brp8963d:~/sar2-d2# python nasa/algorithm/register.py 
Registering algorithm sar2-d2:gcov_pipeline ...
Check registration progress at https://repo.maap-project.org/root/register-job-hysds-v4/-/jobs/15232
@chuckwondo
Copy link
Collaborator

This goes back to the fact that I originally wrote everything under the assumption that we are always using a custom conda env, not a pre-made one.

Therefore, I made this the first line of nasa/algorithm/register.py:

#!/usr/bin/env -S bin/conda/run.sh python

That makes use of the conda run script that ensures all commands run within the custom env.

However, given our recent discussions around needing to be able to use a pre-made env, you can now use the pre-made env by specifying a value for the SAR2D2_ENV env var, just like we recently discussed in the context of using the run.sh command from within notebooks (replace pangeo with whatever env name is appropriate):

SAR2D2_ENV=pangeo nasa/algorithm/register.py

This requires that the conda env named by the SAR2D2_ENV var value has maap-py installed.

If you like, I can enhance nasa/algorithm/register.py to automatically create/use a completely separate env that would contain only maap-py (and it's deps), so that it is completely independent from the primary conda env, and there would be no need to specify a value for SAR2D2_ENV as shown above. The "maap-py"-only env would be used only for scripts that make calls to maap-py, such as register.py.

Let me know if you want me to make that enhancement. In the meantime, I'll simply update README.md to add the env var in front of the command.

@chuckwondo
Copy link
Collaborator

For completeness, here are all of the commands for dealing with algos and jobs as described in the README.md:

nasa/algorithm/register.py
nasa/job/submit.py CALIBRATION_FILE LEFT BOTTOM RIGHT TOP  # Needs to be adjusted with new args
nasa/job/status.py JOB_ID
nasa/job/result.py JOB_ID

Simply place SAR2D2_ENV=<conda-env> in front of each command to get them to use the specified conda env, which must have maap-py installed in it.

Again, I'll update the README.md accordingly, but I can make the enhancement described in the previous comment such that all of these maap-py related commands simply use an independent (auto-created) env that has only maap-py installed, and there would be no need to set the SAR2D2_ENV env var.

@nemo794
Copy link
Collaborator

nemo794 commented Nov 22, 2024

Hi @chuckwondo , no need to update the code right now. I've been running into issues getting everything to play nicely with the new strategy of setting the environment variable, but it's not important for today to spend time cleaning them up right now. For this week, I'm just going to be hacking at the code until I can get it to produce an output, which will get messy. Having both of us push commits to the same branch w/o PR review is going to cause merge conflicts.

After the holidays, let's circle back to this discussion, and figure out a sustainable, more user-friendly strategy to move forward. Thinking of the big-picture, for the good of the project and users, if users need to include these extra boilerplate helper functions into their algorithms to interact with the platform, then that is an indication to me of shortcomings of the MAAP API/platform, which should be resolved at that level.

@chuckwondo
Copy link
Collaborator

Having both of us push commits to the same branch w/o PR review is going to cause merge conflicts.

I'm not suggesting that I push to the same branch. My recent push to the same branch was an accident. I meant to create a branch off of yours and the create a PR. Again, apologies, but fortunately the changes were very minimal, so hopefully not at all disruptive.

After the holidays, let's circle back to this discussion, and figure out a sustainable, more user-friendly strategy to move forward. Thinking of the big-picture, for the good of the project and users, if users need to include these extra boilerplate helper functions into their algorithms to interact with the platform, then that is an indication to me of shortcomings of the MAAP API/platform, which should be resolved at that level.

I heartily agree that there is quite a bit that the platform team can do to make things more user-friendly, and that we should not require a bunch of "helper" scripts included in every algo repo. IMO, there should be a maap-py CLI that implements all of these things.

@nemo794
Copy link
Collaborator

nemo794 commented Nov 22, 2024

IMO, there should be a maap-py CLI that implements all of these things.

Love it. :) As an extension of this exercise, it seems you have identified a list of needs!

@chuckwondo
Copy link
Collaborator

IMO, there should be a maap-py CLI that implements all of these things.

Love it. :) As an extension of this exercise, it seems you have identified a list of needs!

I actually already implemented much of it a few months ago, but haven't managed to circle back to pushing an initial version of it. :-)

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

No branches or pull requests

3 participants