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

changes 5 #434

Merged
merged 69 commits into from
Jun 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
69 commits
Select commit Hold shift + click to select a range
cd9fb9d
Add endpoint for user to check whether they have access to an endpoint
cosa65 May 5, 2023
242c0fa
Merge branch 'master' into 2390-improve-explorer-ux
cosa65 May 11, 2023
2475a1f
Add test
cosa65 May 12, 2023
ebb6f51
Add test
cosa65 May 12, 2023
254359e
can delete parent experiment and explorers can subset an experiment
StefanBabukov May 18, 2023
8243204
WIP - Add cloneDeep to experiment controller that clones most tables …
cosa65 May 18, 2023
a177b85
fix
StefanBabukov May 19, 2023
a7febb3
fix
StefanBabukov May 19, 2023
766c63c
Add plot sql rows copy with reuse of s3 plot data so we avoid some ex…
cosa65 May 19, 2023
575cd80
Add new copy pipeline to copy the s3 files to new exp
cosa65 May 19, 2023
34d8d8f
added new field to spec
StefanBabukov May 19, 2023
79d2ef1
Merge branch 'master' into create-subset-explorer
StefanBabukov May 25, 2023
795e456
Merge pull request #146 from biomage-org/create-subset-explorer
StefanBabukov May 25, 2023
a672d15
temporarily disable admin enforcement
StefanBabukov May 25, 2023
10e57cb
admin enforcement in master
StefanBabukov May 25, 2023
57d7de0
added if
StefanBabukov May 25, 2023
52e05a1
Merge pull request #149 from biomage-org/admin-enforcement
StefanBabukov May 25, 2023
d999792
Add sql migration for plot to only trigger s3 delete lambda if the de…
cosa65 May 26, 2023
305433e
Add share error handling in the form of user messages that explained …
cosa65 May 26, 2023
3f43ee1
Change to handle actual end user messages in the ui
cosa65 May 26, 2023
adf203f
Merge pull request #150 from biomage-org/add-share-error-handling
cosa65 May 26, 2023
f98bfa4
update issue check regex
aerlaut May 30, 2023
0b5ef5b
Merge pull request #152 from biomage-org/add-github-pr-regex
aerlaut May 30, 2023
fa84aed
Merge branch 'master' into 87-clone-exp-w-annot
cosa65 Jun 1, 2023
bc7c52d
Fix, make sure that the new created sample ids sort() result matches …
cosa65 Jun 1, 2023
fad83f9
Comment out failing tests to set up staging
cosa65 Jun 1, 2023
64ada7a
Remove comment
cosa65 Jun 1, 2023
7a80665
Remove old clone experiment controller
cosa65 Jun 1, 2023
6cd3145
Move log
cosa65 Jun 1, 2023
fcab15f
Add plots invalidation to avoid sample ids from parent experiment bei…
cosa65 Jun 1, 2023
946d24b
remove sns from post-registration handler
aerlaut Jun 2, 2023
9e41ee4
skip test to test in staging
aerlaut Jun 2, 2023
fd3a03a
log body and return 200
aerlaut Jun 2, 2023
cb48ee0
remove console.log
aerlaut Jun 2, 2023
7c38d8b
Update test hook run calls with new params
cosa65 Jun 2, 2023
2d7078b
Cleanup BasicModel copy
cosa65 Jun 2, 2023
140324e
Add error for processName not recognized
cosa65 Jun 2, 2023
4b0769f
Add handling for error when copy tries to run but the experiment is b…
cosa65 Jun 2, 2023
8ab8759
Revert "skip test to test in staging"
aerlaut Jun 2, 2023
a20a6e0
fix test for post registration handler
aerlaut Jun 2, 2023
87adfe5
Minor improvement
cosa65 Jun 2, 2023
0ba1241
Add handling to quit early if the original expriment never ran gem2s
cosa65 Jun 2, 2023
6b4a05a
Update clone test and correct a different test (it was testing parts …
cosa65 Jun 2, 2023
ba6c8de
Add test for clone when original exp didnt run gem2s
cosa65 Jun 2, 2023
ffe2807
Update test for custom name clone
cosa65 Jun 2, 2023
30f09aa
Uncomment test
cosa65 Jun 2, 2023
b37edbf
Add test for failure when exp is urnning a pipeline
cosa65 Jun 2, 2023
c89dcf4
Add test for copy pipeline
cosa65 Jun 2, 2023
3ba9822
Uncomment and update model/Experiment tests
cosa65 Jun 2, 2023
35d8c29
Rename createCopy model methods to copyTo for consistency
cosa65 Jun 2, 2023
70641b4
Add test for model/ExperimentExecution.copyTo
cosa65 Jun 2, 2023
d739c31
Add snapshot name
cosa65 Jun 2, 2023
ecdc54e
Add test for plot copyTo
cosa65 Jun 2, 2023
dcab1bc
add encryption to post-register handler
aerlaut Jun 5, 2023
ca6c405
Revert "add encryption to post-register handler"
aerlaut Jun 8, 2023
d4dabdf
Merge branch 'master' into 2390-improve-explorer-ux
cosa65 Jun 8, 2023
5daa0fd
Merge pull request #153 from biomage-org/remove-sns-from-post-registr…
aerlaut Jun 9, 2023
84ad994
Merge branch 'master' into 2390-improve-explorer-ux
cosa65 Jun 9, 2023
ac53e56
Merge pull request #143 from biomage-org/2390-improve-explorer-ux
cosa65 Jun 9, 2023
59b0f13
change install command
aerlaut Jun 9, 2023
307c600
Merge branch 'master' into 87-clone-exp-w-annot
cosa65 Jun 9, 2023
2893f65
Merge pull request #154 from biomage-org/change-make-install-command
aerlaut Jun 9, 2023
3b77f89
Merge branch 'master' into 87-clone-exp-w-annot
cosa65 Jun 9, 2023
d3ee203
Merge pull request #151 from biomage-org/87-clone-exp-w-annot
cosa65 Jun 9, 2023
ce5a223
Merge remote-tracking branch 'biomage/master' into biomage-changes-5
alexvpickering Jun 12, 2023
84d26a0
use last_pipeline_params
alexvpickering Jun 13, 2023
7ac2421
fix tests
alexvpickering Jun 14, 2023
7b9bb05
remove refs
alexvpickering Jun 14, 2023
3a21de0
remove refs
alexvpickering Jun 20, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,6 @@ jobs:
REF_ID: ${{ needs.build-docker.outputs.ref-id }}
IMAGE_TAG: ${{ needs.build-docker.outputs.image-tag }}
REGISTRY: ${{ steps.login-ecr.outputs.registry }}

- name: Push production/develop template to releases
if:
(matrix.environment == 'production' && github.event_name == 'release' && github.event.action == 'released') || (matrix.environment == 'develop' && github.event_name == 'push')
Expand Down Expand Up @@ -370,7 +369,17 @@ jobs:
destination_folder: 'staging-candidates/${{ steps.fill-metadata.outputs.deployment-name }}'
user_email: '[email protected]'
user_name: 'Biomage CI/CD'

- id: disable-admin-enforcement
if:
(matrix.environment == 'production' && github.event_name == 'release' && github.event.action == 'released') || (matrix.environment == 'develop' && github.event_name == 'push')
name: Temporarily disable admin enforcement
uses: benjefferies/branch-protection-bot@master
with:
access_token: ${{ secrets.API_TOKEN_GITHUB }}
owner: ${{ github.repository_owner }}
repo: iac
enforce_admins: false
retries: 8
- name: Push migrations into iac
if:
(matrix.environment == 'production' && github.event_name == 'release' && github.event.action == 'released') || (matrix.environment == 'develop' && github.event_name == 'push')
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/pr_validate.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ jobs:
- id: check-issue-url
name: Check issue URL
run: |-
REGEX="#### URL to issue\s?\r\n(https:\/\/biomage\.atlassian\.net\/browse\/BIOMAGE-[0-9]+|N\/A)"
REGEX="#### URL to issue\s?\r\n(https:\/\/github\.com\/hms-dbmi-cellenics\/issues\/issues\/[0-9]+|N\/A)"

echo "REGEX to test against:"
echo $REGEX
Expand All @@ -41,7 +41,7 @@ jobs:
- id: extract-staging
name: Check staging URL
run: |-
REGEX="#### Link to staging deployment URL \(or set N\/A\)\s?\r\n(https:\/\/ui-(.+)\.scp-staging\.biomage\.net|N\/A)"
REGEX="#### Link to staging deployment URL \(or set N\/A\)\s?\r\n(https:\/\/ui-(.+)\.staging\.single-cell-platform\.net|N\/A)"

echo "REGEX to test against:"
echo $REGEX
Expand Down
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
# Targets
#--------------------------------------------------
install: ## Installs node dependencies
@npm install
@rm -rf node_modules && npm ci
check: ## Checks code for linting/construct errors
@echo "==> Checking if files are well formatted..."
@npm run lint
Expand Down
2 changes: 2 additions & 0 deletions src/api.v2/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const GEM2S_PROCESS_NAME = 'gem2s';
const SEURAT_PROCESS_NAME = 'seurat';
const OLD_QC_NAME_TO_BE_REMOVED = 'pipeline';
const SUBSET_PROCESS_NAME = 'subset';
const COPY_PROCESS_NAME = 'copy';

// Pipeline task names
const ASSIGN_POD_TO_PIPELINE = 'assignPodToPipeline';
Expand Down Expand Up @@ -52,6 +53,7 @@ module.exports = {
SEURAT_PROCESS_NAME,
OLD_QC_NAME_TO_BE_REMOVED,
SUBSET_PROCESS_NAME,
COPY_PROCESS_NAME,
PIPELINE_ERROR,
HANDLE_ERROR_STEP,
END_OF_PIPELINE,
Expand Down
2 changes: 2 additions & 0 deletions src/api.v2/controllers/__mocks__/accessController.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@ const mockGetUserAccess = jest.fn();
const mockInviteUser = jest.fn();
const mockRevokeAccess = jest.fn();
const mockPostRegistration = jest.fn();
const mockIsUserAuthorized = jest.fn();

module.exports = {
getUserAccess: mockGetUserAccess,
inviteUser: mockInviteUser,
revokeAccess: mockRevokeAccess,
postRegistration: mockPostRegistration,
isUserAuthorized: mockIsUserAuthorized,
};
14 changes: 14 additions & 0 deletions src/api.v2/controllers/accessController.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const postRegistrationHandler = require('../helpers/access/postRegistrationHandl

const OK = require('../../utils/responses/OK');
const getLogger = require('../../utils/getLogger');
const UserAccess = require('../model/UserAccess');

const logger = getLogger('[AccessController] - ');

Expand Down Expand Up @@ -54,9 +55,22 @@ const postRegistration = async (req, res) => {
res.json(OK());
};

const isUserAuthorized = async (req, res) => {
const { params: { experimentId }, query: { url, method }, user: { sub: userId } } = req;

logger.log(`Checking access permissions for ${userId}`);

const result = await (new UserAccess().canAccessExperiment(userId, experimentId, url, method));

logger.log(`Finished, returning access permissions for ${userId}`);

res.json(result);
};

module.exports = {
getUserAccess,
inviteUser,
revokeAccess,
postRegistration,
isUserAuthorized,
};
83 changes: 66 additions & 17 deletions src/api.v2/controllers/experimentController.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,33 @@ const invalidatePlotsForEvent = require('../../utils/plotConfigInvalidation/inva
const events = require('../../utils/plotConfigInvalidation/events');
const getAdminSub = require('../../utils/getAdminSub');
const config = require('../../config');
const ExperimentExecution = require('../model/ExperimentExecution');
const Plot = require('../model/Plot');
const { createCopyPipeline } = require('../helpers/pipeline/pipelineConstruct');
const { OLD_QC_NAME_TO_BE_REMOVED, NOT_CREATED } = require('../constants');
const { RUNNING } = require('../constants');
const { GEM2S_PROCESS_NAME } = require('../constants');
const LockedError = require('../../utils/responses/LockedError');

const logger = getLogger('[ExperimentController] - ');

const translateProcessingConfig = (processingConfig, sampleIdsMap) => (
_.transform(processingConfig, (acc, value, key) => {
// If the key is a sample id, then replace it with the new id
const newKey = sampleIdsMap[key] || key;

// Keep going and translate the rest of the object
acc[newKey] = _.isObject(value)
? translateProcessingConfig(value, sampleIdsMap)
: value;
})
);

const getDefaultCPUMem = (env) => {
switch (env) {
case 'staging':
return { podCPUs: 1, podMemory: 14000 };
// Stop using Batch by default in 'production':
// Stop using Batch by default in 'production':
// return { podCPUs: 2, podMemory: 28000 };
default:
return { podCPUs: null, podMemory: null };
Expand Down Expand Up @@ -166,20 +185,11 @@ const downloadData = async (req, res) => {
res.json(downloadLink);
};


const cloneExperiment = async (req, res) => {
const getAllSampleIds = async (experimentId) => {
const { samplesOrder } = await new Experiment().findById(experimentId).first();
return samplesOrder;
};
const userId = req.user.sub;
const {
params: { experimentId: fromExperimentId },
body: {
samplesToCloneIds = await getAllSampleIds(fromExperimentId),
name = null,
toUserId = userId,
},
body: { toUserId = userId, name },
} = req;

const adminSub = await getAdminSub();
Expand All @@ -188,6 +198,15 @@ const cloneExperiment = async (req, res) => {
throw new UnauthorizedError(`User ${userId} cannot clone experiments for other users.`);
}

const {
[OLD_QC_NAME_TO_BE_REMOVED]: { status: qcStatus },
[GEM2S_PROCESS_NAME]: { status: gem2sStatus },
} = await getExperimentBackendStatus(fromExperimentId);

if (qcStatus === RUNNING || gem2sStatus === RUNNING) {
throw new LockedError('Experiment is currently running a pipeline and can\'t be copied');
}

logger.log(`Creating experiment to clone ${fromExperimentId} to`);

let toExperimentId;
Expand All @@ -197,18 +216,48 @@ const cloneExperiment = async (req, res) => {
await new UserAccess(trx).createNewExperimentPermissions(toUserId, toExperimentId);
});

logger.log(`Cloning experiment samples from experiment ${fromExperimentId} into ${toExperimentId}`);
const { samplesOrder: samplesToCloneIds, processingConfig } = await new Experiment()
.findById(fromExperimentId)
.first();

const cloneSamplesOrder = await new Sample().copyTo(
fromExperimentId, toExperimentId, samplesToCloneIds,
);
const cloneSamplesOrder = await new Sample()
.copyTo(fromExperimentId, toExperimentId, samplesToCloneIds);

// Group together the original and copy sample ids for cleaner handling
const sampleIdsMap = _.zipObject(samplesToCloneIds, cloneSamplesOrder);

const translatedProcessingConfig = translateProcessingConfig(processingConfig, sampleIdsMap);

await new Experiment().updateById(
toExperimentId,
{ samples_order: JSON.stringify(cloneSamplesOrder) },
{
samples_order: JSON.stringify(cloneSamplesOrder),
processing_config: JSON.stringify(translatedProcessingConfig),
},
);

// If the experiment didn't run yet, there's nothing else to update
if (gem2sStatus === NOT_CREATED) {
logger.log(`Finished cloning ${fromExperimentId}, no pipeline to run because experiment never ran`);

res.json(toExperimentId);
return;
}

await new ExperimentExecution().copyTo(fromExperimentId, toExperimentId, sampleIdsMap);
await new Plot().copyTo(fromExperimentId, toExperimentId, sampleIdsMap);

const {
stateMachineArn,
executionArn,
} = await createCopyPipeline(fromExperimentId, toExperimentId, sampleIdsMap);

await new ExperimentExecution().upsert(
{ experiment_id: toExperimentId, pipeline_type: 'gem2s' },
{ state_machine_arn: stateMachineArn, execution_arn: executionArn },
);

logger.log(`Finished cloning experiment ${fromExperimentId}, new experiment's id is ${toExperimentId}`);
logger.log(`Began pipeline for cloning experiment ${fromExperimentId}, new experiment's id is ${toExperimentId}`);

res.json(toExperimentId);
};
Expand Down
1 change: 0 additions & 1 deletion src/api.v2/controllers/gem2sController.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ const runGem2s = async (req, res) => {
const { parentExperimentId = null } = await new ExperimentParent()
.find({ experiment_id: experimentId })
.first();

if (parentExperimentId) {
throw new MethodNotAllowedError(`Experiment ${experimentId} can't run gem2s`);
}
Expand Down
12 changes: 9 additions & 3 deletions src/api.v2/helpers/access/createUserInvite.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ const createUserInvite = async (experimentId, invitedUserEmail, role, inviterUse
userAttributes = await getAwsUserAttributesByEmail(invitedUserEmail);

const invitedUserId = userAttributes.find((attr) => attr.Name === 'sub').Value;
new UserAccess().grantAccess(invitedUserId, experimentId, role);
await new UserAccess().grantAccess(invitedUserId, experimentId, role);
emailBody = buildUserInvitedEmailBody(invitedUserEmail, experimentId, inviterUser);
} catch (e) {
if (e.code !== 'UserNotFoundException') {
Expand All @@ -28,11 +28,17 @@ const createUserInvite = async (experimentId, invitedUserEmail, role, inviterUse

logger.log('Invited user does not have an account yet. Sending invitation email.');

new UserAccess().addToInviteAccess(invitedUserEmail, experimentId, role);
await new UserAccess().addToInviteAccess(invitedUserEmail, experimentId, role);
emailBody = buildUserInvitedNotRegisteredEmailBody(invitedUserEmail, inviterUser);
}

await sendEmail(emailBody);
try {
await sendEmail(emailBody);
} catch (e) {
logger.error(e);
// This message is picked up in the ui and transformed to a nice end user message
throw new Error('NotificationFailure');
}

return OK();
};
Expand Down
22 changes: 4 additions & 18 deletions src/api.v2/helpers/access/postRegistrationHandler.js
Original file line number Diff line number Diff line change
@@ -1,32 +1,18 @@
const parseSNSMessage = require('../../../utils/parseSNSMessage');
const getLogger = require('../../../utils/getLogger');
const { OK } = require('../../../utils/responses');

const UserAccess = require('../../model/UserAccess');
const snsTopics = require('../../../config/snsTopics');

const logger = getLogger('[PostRegistrationHandler] - ');

const postRegistrationHandler = async (req) => {
let data;
let messageType;

try {
const { parsedMessage, msg } = await parseSNSMessage(req, snsTopics.POST_REGISTRATION);
data = parsedMessage;
messageType = msg.Type;
} catch (e) {
logger.error('Parsing initial SNS message failed:', e);
return;
}

// If this is a subscription confirmation, we can just return.
if (messageType !== 'Notification') return;

const { userEmail, userId } = data;
const { userEmail, userId } = req.body;

new UserAccess().registerNewUserAccess(userEmail, userId);

logger.log(`Post registration handled for user ${userId}`);

return OK();
};

module.exports = postRegistrationHandler;
9 changes: 8 additions & 1 deletion src/api.v2/helpers/pipeline/gem2s.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ const getLogger = require('../../../utils/getLogger');

const { qcStepsWithFilterSettings } = require('./pipelineConstruct/qcHelpers');
const { getPipelineParams, formatSamples } = require('./shouldPipelineRerun');
const invalidatePlotsForEvent = require('../../../utils/plotConfigInvalidation/invalidatePlotsForEvent');
const events = require('../../../utils/plotConfigInvalidation/events');

const logger = getLogger('[Gem2sService] - ');

Expand Down Expand Up @@ -122,8 +124,13 @@ const setupSubsetSamples = async (payload) => {
// Add samples that were created
};

const invalidatePlotsForExperiment = async (payload, io) => {
await invalidatePlotsForEvent(payload.experimentId, events.CELL_SETS_MODIFIED, io.sockets);
};

hookRunner.register('subsetSeurat', [setupSubsetSamples]);
hookRunner.register('uploadToAWS', [continueToQC]);
hookRunner.register('copyS3Objects', [invalidatePlotsForExperiment]);

hookRunner.registerAll([sendNotification]);

Expand Down Expand Up @@ -232,7 +239,7 @@ const handleGem2sResponse = async (io, message) => {
// Fail hard if there was an error.
await validateRequest(message, 'GEM2SResponse.v2.yaml');

await hookRunner.run(message);
await hookRunner.run(message, io);

const { experimentId } = message;

Expand Down
2 changes: 1 addition & 1 deletion src/api.v2/helpers/pipeline/handleQCResponse.js
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ const handleQCResponse = async (io, message) => {

await validateRequest(message, 'PipelineResponse.v2.yaml');

await hookRunner.run(message);
await hookRunner.run(message, io);

const { experimentId, input: { sampleUuid, taskName } } = message;
const { error = false } = message.response || {};
Expand Down
6 changes: 3 additions & 3 deletions src/api.v2/helpers/pipeline/hooks/HookRunner.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class HookRunner {
}

// run requires taskName to be present as a key in the payload
async run(payload) {
async run(payload, io) {
// run all hooks inside a try catch because they are side-effects and as such, they should
// not break the main program execution
try {
Expand All @@ -45,13 +45,13 @@ class HookRunner {
for (let i = 0; this.hooks[taskName] !== undefined && i < this.hooks[taskName].length; i += 1) {
// calling the hooks sequentially since they may depend on each other
// eslint-disable-next-line no-await-in-loop
this.results[taskName].push(await this.hooks[taskName][i](payload));
this.results[taskName].push(await this.hooks[taskName][i](payload, io));
}

// Runs hooks that apply to all tasks (assigning the results to current task)
for (let i = 0; this.hooks[ALL] !== undefined && i < this.hooks[ALL].length; i += 1) {
// eslint-disable-next-line no-await-in-loop
this.results[taskName].push(await this.hooks[ALL][i](payload));
this.results[taskName].push(await this.hooks[ALL][i](payload, io));
}

logger.log(`Completed ${this.results[taskName].length} hooks for pipeline task ${taskName}`);
Expand Down
Loading