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

Copernicus marine data retrieving tool #126

Closed
wants to merge 60 commits into from

Conversation

Marie59
Copy link
Collaborator

@Marie59 Marie59 commented Sep 6, 2024

Hi @bgruening and @yvanlebras !!

I just finished trying out this new tool to directly retrieve data from the Copernicus Marine Data Store (that goes along @annefou works on atmosphere and climate stores)

I tested it with my own credentials it works fine. Now, I don't know how to test it by setting up the credentials in the user > preferences > manage information part through planemo.

I tried to reuse both Anne's work and the work that was done on another copernicus marine tool medenv (I am not sure if the configfile part is necessary here I kept it from the medenv tool)

Let me know what you think of it !!
Thanks

@Marie59
Copy link
Collaborator Author

Marie59 commented Sep 6, 2024

Oh it went wrong really quickly .... It's an issue with the .shed.yml ?

Copy link
Collaborator

@bgruening bgruening left a comment

Choose a reason for hiding this comment

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

  • should this tool be in its own folder?

echo 'Error. Set your CMEMS credentials via: User -> Preferences -> Manage Information' &&
#end if

eval '$input_text' --force-download --username $cmems_username --password $cmems_password
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is super dangerous, people can put it other commands that gets execute and potentially remove your harddrive I assume.

Why not doing a classical tool with copernicusmarine subset

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

uhoh ...

Yes this is totally possible but less easy for user. I was trying to reproduce the same thing than Anne here https://usegalaxy.eu/root?tool_id=toolshed.g2.bx.psu.edu/repos/climate/cads/cads/0.1.0
But wihtout the file option because the CMEMS website does not offer the file option

Copy link
Collaborator Author

@Marie59 Marie59 Sep 6, 2024

Choose a reason for hiding this comment

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

Would something like that

        #set $command = $input_text.replace("copernicusmarine ", "", 1)

        eval copernicusmarine $command --force-download --username $cmems_username --password $cmems_password 

be better ?
Like that the user can't do anything except a copernicusmarine command no ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are correct. Its also a bit scary in this tool as well. But at least its using literal_eval https://github.com/NordicESMhub/galaxy-tools/blob/de74da48cc04c0cb261d07c451d00598beb834d9/tools/cads/cads_retrieve.py#L37C20-L37C32 and and does some other stripping.

Do you think its feasible to make is less "give me random code and I will execute it"?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What can this "$command" be. How does it usually look like?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The $command would be something like that for instance

 subset --dataset-id cmems_mod_glo_phy-cur_anfc_0.083deg_P1D-m --variable uo --variable vo --start-datetime 2024-09-15T00:00:00 --end-datetime 2024-09-15T00:00:00 --minimum-longitude -180 --maximum-longitude 179.91668701171875 --minimum-latitude -80 --maximum-latitude 90 --minimum-depth 0.49402499198913574 --maximum-depth 0.49402499198913574

<data name="output_netcdf" label="Copernicus marine data" from_work_dir="./*.nc" format="netcdf"/>
</outputs>
<tests>
<test expect_num_outputs="1">
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could test this tools with ENV variables, that we define here in GA. Means we can store your credentials in Github and use them.

The other option is that we tell Galaxy that we know this test is failing and then we check for a specific error message. If this error message pops up the test passed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure I fully understand how to do that but I think the 1st option might be best ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

tools/ocean/copernicusmarine.xml Outdated Show resolved Hide resolved
tools/ocean/divandfull.xml Outdated Show resolved Hide resolved
@Marie59
Copy link
Collaborator Author

Marie59 commented Sep 20, 2024

Its green, it was just a hiccup. @Marie59 I still think its a tool that deserves a separate folder? Its not a suite correct?

No, indeed it's not a suite. But it's all on the same thematic. If I change folders i'll need to close and open a new PR ? we finish here and then I do that ?

@Marie59
Copy link
Collaborator Author

Marie59 commented Oct 1, 2024

So I put my secrets as github variables and it does not seem to work so I am guessing I did not do it right ?

tools/ocean/copernicusmarine.xml Outdated Show resolved Hide resolved
tools/ocean/check.py Outdated Show resolved Hide resolved
tools/ocean/check.py Outdated Show resolved Hide resolved
return False, message
else:
return False, "Error: Command must be 'subset' or 'get'"

Copy link
Collaborator

Choose a reason for hiding this comment

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

What if a user puts in any of --force-download --username --password ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah let's try I'll see and add the correct messages

Copy link
Collaborator Author

@Marie59 Marie59 Oct 8, 2024

Choose a reason for hiding this comment

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

Just tried this is no issue if the user writes down this too it works fine admittedly that the user wrote his credentials in the environment. if not It says the password or username is invalid. Is this enough ?

tools/ocean/copernicusmarine.xml Outdated Show resolved Hide resolved
@Marie59
Copy link
Collaborator Author

Marie59 commented Oct 18, 2024

@bgruening Anything else I can improve ?

@bgruening
Copy link
Collaborator

but tests are failing ...

@Marie59
Copy link
Collaborator Author

Marie59 commented Oct 21, 2024

Yes thts' because I don't know how to correctly put my secret I think no ? I tried but it's not sucessfull

@annefou
Copy link
Contributor

annefou commented Oct 21, 2024

Yes thts' because I don't know how to correctly put my secret I think no ? I tried but it's not sucessfull

How have you done it?

You need to define two variables in your repo: Settings --> Environments CMEMS_USERNAME and CMEMS_PASSWORD and also use them in your GitHub workflow. I don't see anything in your GitHub workflow. Where did you add them?

@Marie59
Copy link
Collaborator Author

Marie59 commented Oct 21, 2024

Yes thts' because I don't know how to correctly put my secret I think no ? I tried but it's not sucessfull

How have you done it?

You need to define two variables in your repo: Settings --> Environments CMEMS_USERNAME and CMEMS_PASSWORD and also use them in your GitHub workflow. I don't see anything in your GitHub workflow. Where did you add them?

I think that's what I missing the adding in the github workflow
How can I do that ?

Actually @bgruening did you add into the settings of this repo ? because I can add for my repo my credentials but I don't have the right here ... or @yvanlebras can you check ?

@annefou
Copy link
Contributor

annefou commented Oct 21, 2024

Ok. Someone having admin of this repo needs to set the environment variables (but since you are using your own credentials, it would be better to give you admin access; I cannot do it on this repo).

For the workflows, I think you need to add the variables in the env sections in pr.yaml and ci.yaml (in your repo and branch Marie59:ocean):

env:
  GALAXY_FORK: galaxyproject
  GALAXY_BRANCH: release_23.2
  MAX_CHUNKS: 40
  CMEMS_PASSWORD: ${{ secrets.CMEMS_PASSWORD }}
  CMEMS_USERNAME: ${{ secrets.CMEMS_USERNAME }}

@bgruening bgruening closed this Oct 22, 2024
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.

4 participants