RC Scripts - [merged] #1

Closed ManavalanG closed 1 year ago

In GitLab by @deeptha on Jul 23, 2021, 16:25

_Merges rcscripts -> master

I created scripts for recommended and required functions and aliases, .requiredcrc and .recommendedrc.

Things to check/review:

[x] review the organization of the scripts (all aliases and functions are in their appropriate script)
[x] review documentation on usage

How this was tested: I exported the scripts from /data/project/worthey_lab/projects/experimental_pipelines/deeptha and tested each alias and function individually to ensure they work. All aliases and function successfully worked.

In GitLab by @deeptha on Jul 23, 2021, 16:25

requested review from @ManavalanG

In GitLab by @deeptha on Jul 23, 2021, 16:25

@wilkb777 can you please review this MR?

In GitLab by @wilkb777 on Jul 27, 2021, 11:18

Commented on rc_scripts/.recommendedrc line 31

alias interbig="srun --ntasks=1 --cpus-per-task=4 --mem-per-cpu=4096 --time=02:00:00 --partition=interactive --job-name=cgd_inter --pty /bin/bash"
alias intersmall="srun --ntasks=1 --cpus-per-task=1 --mem-per-cpu=2048 --time=02:00:00 --partition=interactive --job-name=cgds_inter --pty /bin/bash"

I think changing to the prefix inter is a good idea because inode is a term that is commonly used in file system architecture which @ManavalanG has noted to me in the past could mess with folks in remembering the use of these aliases.

In GitLab by @wilkb777 on Jul 27, 2021, 11:18

Commented on rc_scripts/.recommendedrc line 35

I'm curious what's the use of these aliases? Is there a benefit I'm missing?

In GitLab by @wilkb777 on Jul 27, 2021, 11:18

Commented on rc_scripts/.requiredrc line 17

can we shorten this alias? maybe something like exppipes or exp_pipes to make it easier to quickly type out without making a mistake?

In GitLab by @wilkb777 on Jul 27, 2021, 12:38

I'd recommend that the commands section in the README be reorganized as a table for clarity. Either as an HTML table like

<table>
    <tr>
        <th>Command</th>
        <th>Description</th>
    </tr>
    <tr>
        <td>`SC -j &lt;job_id&gt;`</td>
        <td>blah blah blah</td>
    </tr>
    <tr>
        <td>my_cmd</td>
        <td></td>
    </tr>
</table>

that would render like

Command	Description
`SC -j <job_id>`	blah blah blah
my_cmd

or something in markdown like

| Command          | Description    |
|------------------|----------------|
| `SC -j <job_id>` | blah blah blah |
| my_cmd           |                |

that would render like

Command	Description
`SC -j <job_id>`	blah blah blah
my_cmd

would be helpful in faster assessment and clear intent.

In GitLab by @wilkb777 on Jul 27, 2021, 12:42

@deeptha first review's done, it's ready for you

In GitLab by @wilkb777 on Jul 27, 2021, 12:44

recommendation: don't generate these tables by hand, use something like https://tableconvert.com/ (it's what I do when making tables for documentation, and what I did for the above examples) to build it for you

In GitLab by @deeptha on Jul 30, 2021, 15:06

Commented on rc_scripts/.recommendedrc line 31

changed this line in version 2 of the diff

In GitLab by @deeptha on Jul 30, 2021, 15:06

added 1 commit

fbf18342 - Apply 1 suggestion(s) to 1 file(s)

Compare with previous version

In GitLab by @deeptha on Jul 30, 2021, 15:55

Commented on rc_scripts/.recommendedrc line 35

changed this line in version 3 of the diff

In GitLab by @deeptha on Jul 30, 2021, 15:55

Commented on rc_scripts/.requiredrc line 17

changed this line in version 3 of the diff

In GitLab by @deeptha on Jul 30, 2021, 15:55

added 1 commit

00f68508 - Added table to readme and changed a few aliases

Compare with previous version

In GitLab by @deeptha on Jul 30, 2021, 15:56

Commented on rc_scripts/.recommendedrc line 35

They were a part of the aliases that Mana uses. I removed them because there is no added benefit to having them.

In GitLab by @deeptha on Jul 30, 2021, 15:56

Commented on rc_scripts/.requiredrc line 17

I changed it to exp_pipes.

In GitLab by @deeptha on Jul 30, 2021, 15:57

The commands sections are both in a table format now.

@deeptha @wilkb777 The more I think about this, the more I am convinced that all the aliases and functions here fall under the recommended category instead of required category. I'm not totally opposed to the current setup, but my concern is that it may end up causing unnecessary confusion and frustration, instead of making things easy. That's why I think strongly recommending them would be better than making them required.

module load git/2.8.0-foss-2016a is the only one I could see as required, as the default git in Cheaha is super old.

I agree that rmi/cpi/mvi makes its usage/purpose explicit, but I personally will stick to alias rm/cp/mv. This way my scripts would still work on other users' environment as one would expect, while allowing me the extra layer of safety. rm/cp/mv are commonly used commands and therefore portability of scripts should not be affected by alias usage.

In GitLab by @wilkb777 on Aug 5, 2021, 21:58

Commented on rc_scripts/README.md line 52

| mvi \<source\> \<target\> | Interactive move when overwriting a file                                                           |

to prevent rendering issues you need to escape angle brackets because \< > has a special meaning for rendering things in Markdown. Currently if left unescaped the text for source and target don't render when looking at this in GitLab.

In GitLab by @wilkb777 on Aug 5, 2021, 21:58

Commented on rc_scripts/README.md line 19

| exp_pipes \<directory_name\> | Change directory to experimental pipelines directory (`/data/project/worthey_lab/projects/experimental_pipelines/<directory_name>`) |

best to escape angle brackets in markdown files to prevent formatting issues when it's rendered.

In GitLab by @wilkb777 on Aug 16, 2021, 07:55

Commented on rc_scripts/.recommendedrc line 2

Question on this, how do you deal with scripts that expect the default rm/cp/mv functionality? For example the CGDS FTP downloader uses rm in the job script used to run the download, doesn't the override of rm -i cause this to hang because the job is running non-interactively?

That will be painful but I probably will get around by changing that alias only for that session/job. There will be occasional hiccups like this, and this solution is no way universally applicable. To me, it provides rm = rm -i saves me from occasional mistakes many a times in an year, which is why I stick with them.

To me, that's the role of aliases. They may sometimes present disadvantages but those may be negligible considering the alternate consequences.

In GitLab by @wilkb777 on Aug 16, 2021, 13:29

Commented on rc_scripts/.recommendedrc line 2

Ok given that explanation, you're still good with leaving the rmi/cpi/mvi here and just advocating that you'll use your own ones in your personal bashrc, correct? If that's the case that totally makes sense and is the real purpose of the bashrc :grin: If that's the case then feel free to resolve this thread.

In GitLab by @wilkb777 on Aug 16, 2021, 13:30

Commented on rc_scripts/README.md line 52

@deeptha just a poke for you to review this follow up change recommendation

In GitLab by @wilkb777 on Aug 16, 2021, 13:30

Commented on rc_scripts/README.md line 19

@deeptha just a poke for you to review this follow up change recommendation

In GitLab by @deeptha on Aug 16, 2021, 13:38

Commented on rc_scripts/README.md line 52

changed this line in version 4 of the diff

In GitLab by @deeptha on Aug 16, 2021, 13:38

added 1 commit

e2177363 - Apply 1 suggestion(s) to 1 file(s)

Compare with previous version

In GitLab by @wilkb777 on Aug 16, 2021, 13:39

Absolutely understand this, and I have thoughts in both directions as well. In looking at it in a practical sense we really have no way of enforcing the use of either of these making any points towards required status moot. We can't stop folks from not including them in their bashrc (or zshrc) and honestly I don't want to play account police :joy: . How about just making a name switch? Maybe something like recommended becomes helpful and then we move required to be recommended?

In GitLab by @deeptha on Aug 16, 2021, 13:39

Commented on rc_scripts/README.md line 19

changed this line in version 5 of the diff

In GitLab by @deeptha on Aug 16, 2021, 13:39

added 1 commit

590ed50b - Apply 1 suggestion(s) to 1 file(s)

Compare with previous version

So here is the problem. alias rm =rm -i is meant to catch mistakes by asking folks to confirm the action. rm with such alias would be easy to adopt than having folks try to remember a new command like rmi. And as rm is heavily used command among folks at any level (except null) of experience, rmi would likely never get used by folks.

My strong suggestion would be to use rm =rm -i, but I'm pretty sure we are now in the splitting-hairs zone on this matter :laughing: So I am good with however you guys decide on this; or even removing it altogether from the suggestions.

I like your suggestion, which is quite practical.

My current thinking is that we will start using custom functions and aliases with pipeline (and other stuff) and document them here; when other folks ask questions about its usage or function, we will just point them to this bashrc script.

So basically point to this script, when folks join the lab or whenever somebody has a question :)

In GitLab by @wilkb777 on Aug 16, 2021, 15:25

Yeah, exactly. Present them with it so they are aware, and then let them decide if/when they need these things.

In GitLab by @wilkb777 on Oct 7, 2021, 14:44

@deeptha could you rename the scripts using my proposed name changes in this comment thread and update the documentation accordingly here as well and then I can review and close this thread once that's changed?

In GitLab by @wilkb777 on Oct 8, 2021, 13:55

Commented on rc_scripts/.recommendedrc line 2

We were able to determine based on info from the bash manual and conventional wisdom on StackExchange that aliases specified in the .bashrc file aren't used by non-interactive shell sessions which minimizes my concerns for aliasing rm to be rm -i. After confirming with @ManavalanG and @deeptha we agreed that adding alias rm="rm -i" is the route to go for safety. @deeptha could you update this and then I'll resolve this issue :smile_cat:

In GitLab by @wilkb777 on Oct 8, 2021, 13:58

after a discussion with @ManavalanG we actually should rename these startup scripts to not include rc in the name to prevent new user confusion with Research Computing (RC). Maybe just helpful_startup_commands and recommended_startup_commands would be good names to go with? @deeptha thoughts?

In GitLab by @deeptha on Oct 11, 2021, 13:31

Commented on rc_scripts/.recommendedrc line 2

changed this line in version 6 of the diff

In GitLab by @deeptha on Oct 11, 2021, 13:31

added 1 commit

22e9ac58 - Changed filenames and changed alias for rm, cp, and mv

Compare with previous version

In GitLab by @deeptha on Oct 11, 2021, 13:40

added 1 commit

832521e0 - fixed typo

Compare with previous version

In GitLab by @deeptha on Oct 11, 2021, 13:59

added 1 commit

80fb10ec - renamed directory from rc_scripts to startup_scripts

Compare with previous version

In GitLab by @deeptha on Oct 11, 2021, 14:11

added 1 commit

2a9c2f04 - fixed typo

Compare with previous version

In GitLab by @deeptha on Oct 11, 2021, 14:37

I renamed the scripts to helpful_startup_commands and recommended_startup_commands. I also renamed the directory from rc_scripts to startup_scripts. I updated the readme to reflect this change.

In GitLab by @deeptha on Oct 11, 2021, 14:37

Commented on rc_scripts/.recommendedrc line 2

I changed the alias for rmi, cpi, and mvi to rm, cp, and mv.

In GitLab by @deeptha on Oct 13, 2021, 14:07

added 1 commit

5af33002 - Updated readme

Compare with previous version

In GitLab by @wilkb777 on Oct 15, 2021, 10:17

marked the checklist item review the organization of the scripts (all aliases and functions are in their appropriate script) as completed

In GitLab by @wilkb777 on Oct 15, 2021, 10:17

marked the checklist item review documentation on usage as completed

In GitLab by @wilkb777 on Oct 15, 2021, 10:20

resolved all threads

In GitLab by @wilkb777 on Oct 15, 2021, 10:20

approved this merge request

In GitLab by @wilkb777 on Oct 15, 2021, 10:57

Commented on startup_scripts/README.md line 52

| rm file              | Interactive remove (overrides default `rm` functionality for safety)                                                                                |
| cp                   | Interactive copy when overwriting a file (overrides default `cp` functionality for safety)                                                          |
| mv \<source\> \<target\> | Interactive move when overwriting a file (overrides default `mv` functionality for safety)                                                          |

In GitLab by @wilkb777 on Oct 15, 2021, 11:11

@deeptha after @ManavalanG review he recommended scontr and SR aliases work only for currently running jobs and not for completed/pending jobs. Mentioning this in description would be helpful.

uab-cgds-worthey / learnings_journal

RC Scripts - [merged] #1