Open ianmkenney opened 9 months ago
So there's a difference between src_install
and project_home
, the latter is the one that we want to enforce for FAIR4RS.
The reason for this distinction, and not enforcing a src_install
, is because early on in the MDAKits process we agreed that we wanted to be able to handle mdakits being submitted that weren't fully packaged - i.e. that sometimes someone might come along with a script in a github repo and that would be ok.
That being said, we should revisit this discussion.
Thanks for the clarification. I didn't know the intended semantic difference between src_install
and project_home
.
However, similar to discussion in https://github.com/MDAnalysis/MDAKits/issues/95#issuecomment-1808959635 : They have to tell us how to install their package or we cannot do our job. We do require installation instructions so they must be able to provide them in a form that we can execute.
Therefore, I am also in favor to make src_install
mandatory.
I recognize that install
is phrased ambiguously
a list of commands to use when installing the latest release of the code
and one could argue that install
should be mandatory with whatever instructions are needed, possibly duplicating what's in src_install
. However, I don't support making install
mandatory because effectively we have already taken this to mean "install as a package". If it keeps this meaning (and I propose that we keep it and re-inforce it because that's what most users would consider "installing") then install
should remain optional. Furthermore, if they provide source installation instructions in install
then it's equally easy to provide them in src_install
.
Btw, what happens in the registry if either install
or src_install
(or both) are missing, @IAlibay ?
Btw, what happens in the registry if either
install
orsrc_install
(or both) are missing, @IAlibay ?
I believe the badges just go "grey" - i.e. "could not run this".
We do require installation instructions so they must be able to provide them in a form that we can execute.
That is a very good point. I'm also convinced, however I think the "looser" requirements came from other folk's views (I think @jbarnoud might have been a proponent of this?). Either way, we should at least rope in @lilyminium and @fiona-naughton on this discussion.
I haven't followed mdakit for a long while so I am lacking context. Bouncing on an earlier comment of Irfan, the two main ideas I had about the project were:
I agree that we want to encourage people to share. Minimal testing is, however, one of those requirements where we drew a line, as spelled out in our requirements for becoming a registered kit https://mdakits.mdanalysis.org/about.html#what-is-an-mdakit . I don't think we want to remove this requirement, do we?
Personally, I do believe in the "untested code is broken code" adage" and I see part of our mission to educate on minimal best practices. From my experience, people are more motivated to go the extra mile when they want to achieve something (like getting a kit registered) as opposed to "coming back later to add tests when I have time".
If we want a two-tiered registry where we have tested kits with green badges and a "second tier" of kits that are effectively just links without badges or red badges then we need to discuss it.
and we can add you to our ci
That's exactly what we're doing, but in order to do it we need to know
@fiona-naughton and @lilyminium please have a look at this issue and the related #95 on making three important parameters in the registration yaml file required. Your comments would be very much appreciated!
The parameters are
source_install
: commmands to install the software from source
run_tests
and test_dependencies
: commands to run tests and to install any deps for the tests
Requirements for a (registered) MDAKit are in the paper and summarized on our web site.
One reason why they are currently optional was related to keeping the barrier to entry low, as expressed in https://github.com/MDAnalysis/MDAKits/issues/98#issuecomment-1808773002 and https://github.com/MDAnalysis/MDAKits/issues/98#issuecomment-1809960174 .
Restarting this discussion having been reminded when writing the reviewer's guide - my apologies for not having commented at the time.
I would tend to agree with Oliver; I understand stuff like writing tests can be a daunting barrier, but I would consider it one of the key aims of MDAKits to be making this barrier as low as possible so we are building up scientists who know how to write better code (with the cookie cutter and instructions, hopefully some additional resources I'm planning to put together, reviewers there to provide advice, the incentive of registering a kit, etc), rather than removing it entirely by making it optional - which just delays the issue (similar to what Oliver said, if that first step isn't now, I'm not sure I see them coming back to take it later). It's also in line with how we've been selling it to users.
(It doesn't need to be super comprehensive tests - the biggest barrier imo is understanding the general idea and format, so as long as they've encountered that, hopefully building out more coverage is a lot less scary and more likely to happen)
(Also, it'll be easier to relax a compulsory rule in the future if we do decide it's too off putting, then have to instate it and need to sort through everything that's already been registered)
I'd like to revive this issue together with #95 and come to a decision. Can we discuss at the next MDAKits meeting?
I put it on the agenda for the 2024-07-17 meeting.
At the EOSS4 MDAKits meeting on Jul 17, the MDAKits team decided that we want to make minimal tests and installation from source MANDATORY and thus enforce it during reviews.
In a subsequent round of consultations with the CoreDevs, there were a number of positive comments and no objections.
Therefore, minimal tests and installation from source instructions are MANDATORY and enforceable during review.
Since the FAIR4RS requires that source is accessible, we should make the
src_install
field required. From there I think it's fair to keepinstall
as an optional field.