Skip to content

added quickchart and mongodb#226

Merged
romeokienzler merged 750 commits into
claimed-framework:mainfrom
RafflesiaKhan:add-mondodb-connection
May 24, 2023
Merged

added quickchart and mongodb#226
romeokienzler merged 750 commits into
claimed-framework:mainfrom
RafflesiaKhan:add-mondodb-connection

Conversation

@RafflesiaKhan
Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

I have added three new scripts

How was this pull request tested?

These files are tested locally, and they are just making connection and printing data I am not certain which kind of test case I can add here, but I am happy to get any suggestions.

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

@RafflesiaKhan RafflesiaKhan force-pushed the add-mondodb-connection branch 2 times, most recently from a876e0d to 7dd0e5c Compare May 15, 2023 11:10
@romeokienzler
Copy link
Copy Markdown
Member

Dear @RafflesiaKhan - first of all, thank you so much for your contributions! Here are some things which need to be done before the new code can be merged, otherwise C3 (the CLAIMED component compiler) will fail to create (docker) container images, kubeflow components and CLI support - which usually happens automatically

  • Please make sure the notebooks are clean (no numbering/output) - this helps in resolving conflicts at a later stage - in jupypter lab you can just select 'clear all outputs' in vsCode ' clear all'

  • A use of a component (which gets generated from your code) ideally does not need to edit to code to be used. Therefore, every configuration items needs to be provided from 'the outside' - if you have a look at this example you will notice that there exists a cell where a lot of environment variables are read. The next cell just does the same for all CLI command line parameters which might be passed to your code - the last cell works for all string type parameters, the first cell (where the env variables are read) you need to adjust to e.g., database url, database name, collection.... - also the document or visialization should not be printed inline but written to a file - the file name should also be specified in that parameter section - please note that output parameter need to start with an 'output_'

@RafflesiaKhan RafflesiaKhan force-pushed the add-mondodb-connection branch from 4e96d58 to 83400f2 Compare May 15, 2023 20:19
@romeokienzler
Copy link
Copy Markdown
Member

Dear @RafflesiaKhan just had a look at your latest commit - we're making progress, please note that the correct way to 'ingest' configuration parameters is by reading the environment variables

so instead of

database_username = 'database_username'

you can write

database_username = os.environ.get('database_username', 'HERE_GOES_A_POTENTIAL_DEFAULT_VALUE')

data_dir is not needed anymore

Please have a look at the template notebook I've provided above, there is a code section which also needs to be added to be able to also 'ingest' configuration parameters via command line arguments

image

@RafflesiaKhan
Copy link
Copy Markdown
Contributor Author

Dear @RafflesiaKhan just had a look at your latest commit - we're making progress, please note that the correct way to 'ingest' configuration parameters is by reading the environment variables

so instead of

database_username = 'database_username'

you can write

database_username = os.environ.get('database_username', 'HERE_GOES_A_POTENTIAL_DEFAULT_VALUE')

data_dir is not needed anymore

Please have a look at the template notebook I've provided above, there is a code section which also needs to be added to be able to also 'ingest' configuration parameters via command line arguments

image

@romeokienzler Thanks you so much for the comments, sorry I missed the code section, will update the PR soon

@RafflesiaKhan RafflesiaKhan force-pushed the add-mondodb-connection branch 2 times, most recently from 8a100e4 to 37e1572 Compare May 16, 2023 14:52
@romeokienzler
Copy link
Copy Markdown
Member

Thanks @RafflesiaKhan for the updates

I think we are almost there

In this file

I think you don't need any default values for the parameters as there are no defaults, an example of a default value would be 5432 as postgesql port or so

so you can change

document = os.environ.get('document', 'HERE_GOES_YOUR_DATABASE_DOCUMENT')

to

document = os.environ.get('document')

...and so on, you can also delete the following:

# temporal data storage for local execution data_dir = os.environ.get('data_dir', '../../data/')

@romeokienzler
Copy link
Copy Markdown
Member

In this file you have two cells with env parameters, can you please move eveything to the first one?

@RafflesiaKhan
Copy link
Copy Markdown
Contributor Author

Hi @romeokienzler , Thank you so much for the details instructions, I have tried to modify the code accordingly, not sure if it's ok now or not, please feel free to add comments for improvements.

I have added 3 scripts and some other files got generated, are those related to the docker image, or it can be me merging incorrectly 🙄🫣 , still learning about the working procedure of the project, please let me if you have any idea about this, in either case, I can try to fix that.

@RafflesiaKhan RafflesiaKhan force-pushed the add-mondodb-connection branch 2 times, most recently from 0d2cf57 to 19adc26 Compare May 17, 2023 08:30
@RafflesiaKhan
Copy link
Copy Markdown
Contributor Author

Hi @romeokienzler, Thank you so much for the detailed comments; those helped a lot; sorry about all these issues; thanks for being patient. I promise future PRs will be much cleaner 🙂. Please feel free to add comments if any other updates are required here.

@RafflesiaKhan RafflesiaKhan force-pushed the add-mondodb-connection branch 5 times, most recently from c569148 to cac1273 Compare May 20, 2023 21:30
@RafflesiaKhan
Copy link
Copy Markdown
Contributor Author

Hi @romeokienzler, sorry got busy with office work, I have removed the # temporal data .. stuff, most of the previous comments are now covered. Please let me know if anything else is required. Thank you.

@romeokienzler romeokienzler self-assigned this May 23, 2023
@romeokienzler
Copy link
Copy Markdown
Member

romeokienzler commented May 23, 2023

Thanks so much @RafflesiaKhan I think you are almost done. The last thing missing is to remove the 2nd parameter of <> calls to the 'get' function in os.environ.get

so instead of

cluster_url = os.environ.get('cluster_url',"HERE_GOES_YOUR_CLUSTER_URL")

please write

cluster_url = os.environ.get('cluster_url')

The 2nd parameter to that 'get' function is only there in case the environment variable is not set that there will be a default value set.

So please go ahead and remove the 2nd parameter in ALL the function calls in ALL the notebooks you have provided and then I think we are good.

One last thing, the CLAIMED component compiler doesn't support multi line comments for the parameters, so instead of

# Input parameters for chart
# chart type
chart_type = os.environ.get('chart_type','HERE_GOES_YOUR_CHART_TYPE')

you should write

# Input parameters for chart, chart type 
chart_type = os.environ.get('chart_type','HERE_GOES_YOUR_CHART_TYPE')

and of course also remove the 2nd parameter

# Input parameters for chart, chart type 
chart_type = os.environ.get('chart_type')

@RafflesiaKhan RafflesiaKhan force-pushed the add-mondodb-connection branch from 92c71f5 to f4a35b4 Compare May 23, 2023 08:01
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
This reverts commit 0fc561e.

Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
…scripts

added mondodb and quickchart scripts

Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
text updated

Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
romeokienzler and others added 25 commits May 23, 2023 14:52
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
Signed-off-by: Rafflesia Khan <rafflesiakhan.nw@gmail.com>
@RafflesiaKhan
Copy link
Copy Markdown
Contributor Author

Hi, @romeokienzler, Thank you. I have removed the 2nd parameter and 2 line comments.

@romeokienzler romeokienzler merged commit 09acf18 into claimed-framework:main May 24, 2023
@romeokienzler
Copy link
Copy Markdown
Member

Thanks so much @RafflesiaKhan - great work - you are now an official contributor to an IBM sponsored Linux Foundation AI project

[x] merged

@RafflesiaKhan
Copy link
Copy Markdown
Contributor Author

Thanks so much @RafflesiaKhan - great work - you are now an official contributor to an IBM sponsored Linux Foundation AI project

[x] merged

Yeaaaaaa 😎. I truly appreciate your encouragement, @romeokienzler! It has been such an amazing experience so far and I'm looking forward to continuing my journey as a contributor. Thank you so much for your support! 🙂

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