-
Notifications
You must be signed in to change notification settings - Fork 3
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
Archives tools prototypes #25
Conversation
necessary and current
future
@francoismg could you check these checkboxes? |
@francoismg I put some checkboxes. Could you see that it makes sense? Please ask questions if unclear or there are any roadblocks! |
just to the record, we might end up using an interactive tool in the future, e.g. like #26 |
Yes everything seem clear right now, I'm just not sure about what you want to add in the citations section. Also the for the tap explorer I'm not sure that we can display something like this directly from a static file but I will try it and let you know. |
yea sure no problem, I thought it was already decided that it would probably be an interactive tool |
We might be talking about different things. Let's discuss later. But for now, let's just finish this first basic but already useful tool and find out with upstream developers how to best fill the gaps. |
we want several things here, specifically: 10.3847/1538-3881/aabc4f etc from https://github.com/astropy/astropy/blob/main/astropy/CITATION for https://github.com/astropy/pyvo I do not see a DOI directly, just put the URL then for https://www.ivoa.net/documents/latest/ADQL.html 10.5479/ADS/bib/2008ivoa.spec.1030O please put these and also a TODO to let me think more what to add
Actually as discussed let's postpone this tap explored until after delivery of the first tool version. I will indicate this in the list. |
…rameter order + changed file output selection + better error handling and reporting + added citations + changed test cases + added basic output html as output choice
<param name="t_min" type="text" label="Start time in MJD" /> | ||
<param name="t_max" type="text" label="Stop time in MJD" /> | ||
<param name="dataproduct_type" type="text" label="Data product type" help="Logical data product type (image etc.)" /> | ||
<param name="dataproduct_type_select" type="select" label="Data product type" help="Logical data product type"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add reference where this comes from
try: | ||
fits_file = FileHandler.download_file_from_url(url[access_url]) | ||
FileHandler.write_file_to_subdir(fits_file, FileHandler.get_file_name_from_url(url[access_url])) | ||
except Exception as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handle this exception with some extra output to the user
access_url = 'access_url' | ||
|
||
fits_file = FileHandler.download_file_from_url(file_url[0][access_url]) | ||
FileHandler.write_file_to_output(fits_file, output, "wb") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
portentially this fails?
</data> | ||
<data name="output_error" format="txt" label="${tool.name} -> Error Log:"></data> | ||
</outputs> | ||
<tests> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add tests with failure, empty archive, no obscore, etc
@@ -0,0 +1,1005 @@ | |||
<tool id="astronomical_archives" name="Astronomical archives" version="1.0.0"> | |||
<description>queries astronomical archives</description> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<description>queries astronomical archives</description> | |
<description>queries astronomical archives through Virtual Observatory protocols</description> |
</param> | ||
<when value="registry"> | ||
<param name="keyword" type="text" label="Keyword" /> | ||
<param name="service_type" type="select" label="Service type"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please put a reference for versionned service type vocabulary. also for all other select options which rely on a particular version of some vocabularies
@francoismg @volodymyrss should I review again? When is your talk, we need to get this installed by then, do we? |
Yes, it would be good. It's next Wednesday. |
When should we aim to install it so that it can appear on time? Assuming it might not workout at once. |
Once its merged I can install it manually. So this can be fast. But I would say Monday latest? |
I reworked how the tool run now that it get the outputs from json, it seems more readable and less clutered that way but took me longer than I thought to fix all the bugs. RIght now everything seems to be working and I have merged the changes from that PR. I am merging the changes from your other PR right now so should be good soon |
</when> | ||
<when value="archive"> | ||
<param name="archive" type="select" label="Astronomical archives"> | ||
<options from_data_table="astronomical_archives_1" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why has this a _1 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my bad thanks, forgot to change after moving the code
Linting is failing because of Python flake8 errors. |
<param name="output_selection" value="i"/> | ||
<param name="number_of_files" value="1"/> | ||
<conditional name="archive_selection"> | ||
<param name="archive_type" value="registry"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be nice to have at least one test using the data_table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had some when I was using "from file" instead of a data_table but when running planemo test I always got some "invalid value" or something like that.
I could try it again now that we moved to data_table
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
</assert_contents> | ||
</output> | ||
</test> | ||
<test expect_num_outputs="1"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mh, the tool does not fail when we have an error in the query?
Would that not be the expected behaviour from a users point of view.
You can test for error code and such. But this needs to be implemented in your python script I guess, that you really fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when a query fail we update the summary output to inform the user so he can possibly fix the error in the query, if the tool failed the user would not get the summary output right or am I mistaken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if it fails the outputs is not returned. But you get the stdout/stderr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I thought it would be more user friendly if we informed the user through some summary file but we could just output everything in the stdout if you think it would be better no problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your call, I don't know :)
I'm happy to +1 it as it is :)
If you merge please create a pr to the tools repo I will make sure it gets installed during your night latest. |
@francoismg you want to add it? Here is an example. |
yea sure will try that |
So just to be sure I understand correctly. I should make a pr to the usegalaxy-eu-tools repo to update the astronomy yaml with the new tool reference and after that push the actual tool files to another repo is that correct? You said you pushed your tools with planemo but that it caused some issues right? So after updating the yaml file I should make a pr to @bgruening repo https://github.com/bgruening/galaxytools or the IUC repo https://github.com/galaxyproject/tools-iuc with the actual tool files, does that make sense? |
Not quite. This tools-astro repository here has CI/CD which pushes to the toolsched. It just failed due to, apparently, toolsched being down?.. It seems back now very slow to respond, let's see if CI completes. Once pushed to toolshed, the tools are added to the galaxy EU instance with esg-epfl-apc/usegalaxy-eu-tools#1 |
ok thanks, so just need to wait then |
Repository [suite_astronomicalarchivestools] does not exist in the targeted Tool Shed. There's also that error, I followed the other yaml file you made and created another suite but apparently that might not be correct |
The problem you quote was with access and I suspect only that, all the following problems follow it. The problem now is this one:
|
I pushed a commit with a loc.sample file but that doesn't seem to work since the pr has been merged (i think), will open another pr |
Tools prototypes for archives browsing