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

Merge the Updated Codebase #201

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Merge the Updated Codebase #201

wants to merge 22 commits into from

Conversation

Linardos
Copy link
Collaborator

No description provided.

Copy link

gitguardian bot commented Oct 13, 2024

⚠️ GitGuardian has uncovered 50 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

Since your pull request originates from a forked repository, GitGuardian is not able to associate the secrets uncovered with secret incidents on your GitGuardian dashboard.
Skipping this check run and merging your pull request will create secret incidents on your GitGuardian dashboard.

🔎 Detected hardcoded secrets in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
- - RSA Private Key 5ae77c9 Task_1/cert/client/col_two.key View secret
- - RSA Private Key 3ba75b2 Task_1/cert/ca/signing-ca/private/signing-ca.key View secret
- - RSA Private Key 3ba75b2 Task_1/cert/ca/root-ca/private/root-ca.key View secret
- - RSA Private Key 295c046 Task_1/cert/server/agg_in-ota-232347.ads.iu.edu.key View secret
- - RSA Private Key 6414e2a Task_1/cert/server/agg_in-ota-232347.ads.iu.edu.key View secret
- - RSA Private Key fbef799 Task_1/cert/ca/root-ca/private/root-ca.key View secret
- - RSA Private Key 5763b01 Task_1/cert/ca/root-ca/private/root-ca.key View secret
- - RSA Private Key 295c046 Task_1/cert/ca/signing-ca/private/signing-ca.key View secret
- - RSA Private Key 6414e2a Task_1/cert/ca/root-ca/private/root-ca.key View secret
- - RSA Private Key 5763b01 Task_1/cert/ca/signing-ca/private/signing-ca.key View secret
- - RSA Private Key a94d2b4 Task_1/cert/ca/signing-ca/private/signing-ca.key View secret
- - RSA Private Key 5763b01 Task_1/cert/client/col_one.key View secret
- - RSA Private Key 7f238e7 Task_1/cert/ca/signing-ca/private/signing-ca.key View secret
- - RSA Private Key a94d2b4 Task_1/cert/server/agg_in-ota-232347.ads.iu.edu.key View secret
- - RSA Private Key 295c046 Task_1/cert/ca/root-ca/private/root-ca.key View secret
- - RSA Private Key a94d2b4 Task_1/cert/client/col_one.key View secret
- - RSA Private Key 5763b01 Task_1/cert/client/col_two.key View secret
- - RSA Private Key 5ae77c9 Task_1/cert/ca/root-ca/private/root-ca.key View secret
- - RSA Private Key a94d2b4 Task_1/cert/client/col_two.key View secret
- - RSA Private Key 6414e2a Task_1/cert/client/col_two.key View secret
- - RSA Private Key 9d932a8 Task_1/cert/ca/root-ca/private/root-ca.key View secret
- - RSA Private Key 6db76ad Task_1/cert/client/col_one.key View secret
- - RSA Private Key 7f238e7 Task_1/cert/client/col_two.key View secret
- - RSA Private Key 9d932a8 Task_1/cert/ca/signing-ca/private/signing-ca.key View secret
- - RSA Private Key 5ae77c9 Task_1/cert/ca/signing-ca/private/signing-ca.key View secret
- - RSA Private Key fbef799 Task_1/cert/ca/signing-ca/private/signing-ca.key View secret
- - RSA Private Key 7f238e7 Task_1/cert/client/col_one.key View secret
- - RSA Private Key 5763b01 Task_1/cert/server/agg_in-ota-232347.ads.iu.edu.key View secret
- - RSA Private Key 295c046 Task_1/cert/client/col_one.key View secret
- - RSA Private Key 6db76ad Task_1/cert/client/col_two.key View secret
- - RSA Private Key 9d932a8 Task_1/cert/client/col_one.key View secret
- - RSA Private Key 3ba75b2 Task_1/cert/client/col_two.key View secret
- - RSA Private Key fbef799 Task_1/cert/client/col_one.key View secret
- - RSA Private Key 6db76ad Task_1/cert/server/agg_in-ota-232347.ads.iu.edu.key View secret
- - RSA Private Key 9d932a8 Task_1/cert/client/col_two.key View secret
- - RSA Private Key fbef799 Task_1/cert/server/agg_in-ota-232347.ads.iu.edu.key View secret
- - RSA Private Key 295c046 Task_1/cert/client/col_two.key View secret
- - RSA Private Key 5ae77c9 Task_1/cert/client/col_one.key View secret
- - RSA Private Key 3ba75b2 Task_1/cert/client/col_one.key View secret
- - RSA Private Key 7f238e7 Task_1/cert/server/agg_in-ota-232347.ads.iu.edu.key View secret
- - RSA Private Key 6db76ad Task_1/cert/ca/signing-ca/private/signing-ca.key View secret
- - RSA Private Key a94d2b4 Task_1/cert/ca/root-ca/private/root-ca.key View secret
- - RSA Private Key 6414e2a Task_1/cert/client/col_one.key View secret
- - RSA Private Key 5ae77c9 Task_1/cert/server/agg_in-ota-232347.ads.iu.edu.key View secret
- - RSA Private Key 3ba75b2 Task_1/cert/server/agg_in-ota-232347.ads.iu.edu.key View secret
- - RSA Private Key 9d932a8 Task_1/cert/server/agg_in-ota-232347.ads.iu.edu.key View secret
- - RSA Private Key fbef799 Task_1/cert/client/col_two.key View secret
- - RSA Private Key 6db76ad Task_1/cert/ca/root-ca/private/root-ca.key View secret
- - RSA Private Key 7f238e7 Task_1/cert/ca/root-ca/private/root-ca.key View secret
- - RSA Private Key 6414e2a Task_1/cert/ca/signing-ca/private/signing-ca.key View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Copy link
Member

@sarthakpati sarthakpati left a comment

Choose a reason for hiding this comment

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

@psfoley do you see any major changes preventing us from merging this ASAP?

@psfoley
Copy link
Collaborator

psfoley commented Oct 15, 2024

@psfoley do you see any major changes preventing us from merging this ASAP?

@sarthakpati Nope - I would expect this can be merged without issue.

@sarthakpati
Copy link
Member

@psfoley do you see any major changes preventing us from merging this ASAP?

@sarthakpati Nope - I would expect this can be merged without issue.

Cool, thanks for the confirmation.

@Linardos: thank you once again for the herculean effort that you and @kta-intel have put together to modernize this codebase. Once you fix the comments that I have, I can start putting together some CI/CD stuff to make our lives much easier.

@Linardos
Copy link
Collaborator Author

Linardos commented Nov 6, 2024

@sarthakpati I addressed your comments. You may merge. Note that a version of generate_predictions for classification (i.e. inference for classification) still needs work to function though. I can do a separate PR for that when I have it

@sarthakpati
Copy link
Member

There are some unresolved conversations, and I am unable to merge until they are resolved. Also, does this PR contain #198 as well?

@Linardos
Copy link
Collaborator Author

Linardos commented Nov 7, 2024

Seems #198 is an outdated part of the PR that I have since then removed because csv files are generated on-the-go. I resolved the conversations. I also made the code default to segmentation which is functional through both training and inference. Classification will still need some work in terms of inferring the metrics, but we can do that in a later PR.

@sarthakpati sarthakpati mentioned this pull request Nov 8, 2024
.gitignore Outdated Show resolved Hide resolved
Task_1/FeTS_Challenge.py Show resolved Hide resolved
Task_1/README.md Outdated Show resolved Hide resolved
@@ -28,11 +28,11 @@
],
include_package_data=True,
install_requires=[
'openfl @ git+https://github.com/intel/openfl.git@f4b28d710e2be31cdfa7487fdb4e8cb3a1387a5f',
'GANDLF @ git+https://github.com/CBICA/GaNDLF.git@e4d0d4bfdf4076130817001a98dfb90189956278',
'openfl @ git+https://github.com/securefederatedai/openfl.git@kta-intel/fets-2024-patch-1',
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be problematic:

image

Is there any plan to merge this with openfl's develop branch so that we can use that instead? This seems prone to breaking, right? Tagging @kta-intel for guidance.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just opened a PR for it: securefederatedai/openfl#1141
Once it is merged, we can delete that branch and update the setup.py to use the develop branch directly

Couple notes:

  1. I moved the conditional into loader_gandlf.py instead of runner_gandalf.py since it is logically a better spot to check if we are running in inference mode, rather than setting another. Simple move that tests fine on my end, but be mindful that it doesn't affect anything else on your end
  2. The native API (i.e. import openfl.native as fx) will likely be deprecated at some point in this or an upcoming release, so it might be a good idea to pin to this specific openfl commit rather than develop as a whole

Copy link
Member

Choose a reason for hiding this comment

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

Point 2 is very important. Are there plans to preserve backward compatibility for a few versions?

Copy link
Collaborator

@kta-intel kta-intel Nov 12, 2024

Choose a reason for hiding this comment

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

It has been merged

How to cleanly handle Point 2 is still being discussed. Considering the importance of the FeTS challenge, let me open up the discussion and propose a way to preserve it at least for v1.7

Copy link
Member

@sarthakpati sarthakpati Nov 13, 2024

Choose a reason for hiding this comment

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

propose a way to preserve it at least for v1.7

This would be a good solution. Ideal would be for this API to be supported (with a deprecation warning) until at least 1.9 (considering the conventions of semantic versioning).

'openfl @ git+https://github.com/intel/openfl.git@f4b28d710e2be31cdfa7487fdb4e8cb3a1387a5f',
'GANDLF @ git+https://github.com/CBICA/GaNDLF.git@e4d0d4bfdf4076130817001a98dfb90189956278',
'openfl @ git+https://github.com/securefederatedai/openfl.git@kta-intel/fets-2024-patch-1',
'GANDLF @ git+https://github.com/CBICA/GaNDLF.git@0.1.0',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'GANDLF @ git+https://github.com/CBICA/[email protected]',
'GANDLF @ git+https://github.com/mlcommons/[email protected]',

Can we open an issue where we add a CI run on both GaNDLF and OpenFL master/develop branches respectively?

import pickle

# Path to the pickle file
pickle_file_path = '/home/locolinux2/.local/workspace/checkpoint/experiment_109/best_model.pkl'
Copy link
Member

Choose a reason for hiding this comment

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

This path (and anything similar) is going to be problematic. I will suggest using something that defined based on pathlib.Path.cwd() (or something similar) to ensure non-breakage.

Comment on lines +527 to +532
institution_split_csv_filename = '/home/locolinux2/datasets/MICCAI_FeTS2022_TrainingData/partitioning_2.csv'
institution_split_csv_filename = '/home/locolinux2/datasets/MICCAI_FeTS2022_TrainingData/sanity_partitioning.csv' # a small subset for sanity checks and debugging. Comment out to run the actual challenge partition.

# change this to point to the parent directory of the data
brats_training_data_parent_dir = '/raid/datasets/FeTS22/MICCAI_FeTS2022_TrainingData'
brats_training_data_parent_dir = '/home/locolinux2/datasets/MICCAI_FeTS2022_TrainingData'
brats_training_data_parent_dir = '/home/locolinux2/datasets/MICCAI_FeTS2022_Resized'
Copy link
Member

Choose a reason for hiding this comment

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

These paths (and anything similar) is going to be problematic. I will suggest using something that defined based on pathlib.Path.cwd() (or something similar) to ensure non-breakage.

- User inside singularity containers isn't root: This can lead to `PermissionError` when reading files from the file system like model checkpoints. Make sure that all files that need to be read from inside the container can be read by *all users*, either before copying them in the Dockerfile or adding chmod commands to the Dockerfile.

Any other Errors ? Feel free to contact us: [forum](https://www.synapse.org/#!Synapse:syn28546456/discussion/default)
Task 2 is still available on this link: https://github.com/FeTS-AI/Challenge/tree/2022/Task_2
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Task 2 is still available on this link: https://github.com/FeTS-AI/Challenge/tree/2022/Task_2
Information related to Task 2 of the FeTS Challenge is available on this link: https://github.com/FeTS-AI/Challenge/tree/2022/Task_2

@sarthakpati
Copy link
Member

Hey @Linardos, apologies for the late review (as you know, I have been swamped). Anyway, great work on all the changes. I am requesting a few minor points of consideration before we can merge this to be used by others. Thanks!

@sarthakpati
Copy link
Member

@Linardos since the PR from @kta-intel has been merged (securefederatedai/openfl#1141), can you please update the setup appropriately?

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