-
Notifications
You must be signed in to change notification settings - Fork 119
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
Redesign Build Deployment Process (External) #961
Changes from all commits
93c2232
a80d7e4
20e1856
4a1005e
d4dad4a
02c7fc7
4aeb4a6
2531672
ea57afa
e8344e9
7f0d5f0
05551c8
cabc98a
c39b537
385bc97
273c2ca
2fdb469
91abdea
bbdaaae
5d0ca02
2e8a911
f5aae47
051ef66
6c44865
4569dfb
b562c2c
25109b3
709f3cf
4b42185
12d09ae
94c129b
0dd1245
5434914
0fd3e9d
ab399ea
91c0c1f
acecf3e
e778b3f
a0190d4
776f0b9
17ac3cc
706f74c
d724fd1
e6a2d79
4b3dfdf
f033f06
1207d79
f306e8c
00d9565
f182790
f1869ab
8f05955
912bd34
738b629
7102508
3d77439
a0f2424
9fb0e5a
18a8872
53015c7
41a410c
7405ff1
cd62247
ebc8188
41ae79f
290b0fc
29869cd
38209ef
da485c2
e6b388b
bb03c42
cf50d17
6a21f5d
b961417
a245e7d
2067055
3000758
6a8a13f
fc183cb
51e16de
7ab37b6
727c00c
7f1be92
7177e71
168ef10
3dea305
a0f0c6a
10624a6
0fe9476
11d2a89
4cc4c58
357d4b8
b6f59b0
ec38835
1a0d451
2fe0816
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
{ | ||
"intake.segmentation.section_segmentation.sectionValidityAssertions": true, | ||
"intake.cleaning.clean_and_resample.speedDistanceAssertions": false, | ||
"intake.cleaning.clean_and_resample.sectionValidityAssertions": false, | ||
"intake.cleaning.filter_accuracy.enable": false, | ||
"classification.inference.mode.useAdvancedFeatureIndices": true, | ||
"classification.inference.mode.useBusTrainFeatureIndices": true, | ||
"classification.validityAssertions": true, | ||
"output.conversion.validityAssertions": true, | ||
"section.startStopRadius": 150, | ||
"section.endStopRadius": 150, | ||
"analysis.result.section.key": "analysis/inferred_section", | ||
"userinput.keylist": ["manual/mode_confirm", "manual/purpose_confirm", "manual/replaced_mode", "manual/trip_user_input", "manual/place_user_input"] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,17 @@ | ||
import json | ||
import os | ||
|
||
def get_config_data(): | ||
try: | ||
print("Trying to open debug.conf.json") | ||
config_file = open('conf/analysis/debug.conf.json') | ||
Comment on lines
+6
to
7
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not fully convinced that this is the right approach. If we keep this backwards compat code around forever, we don't have any motivation for people to change to the new structure in the future. Having said that, I will not hold up the merge for this, but I do want to make sure that we have a plan to remove this in a year or so, and to notify users to change their config files, so that we don't end up with a bunch of hacky backwards compat code strewn all around the codebase. |
||
except: | ||
print("analysis.debug.conf.json not configured, falling back to sample, default configuration") | ||
config_file = open('conf/analysis/debug.conf.json.sample') | ||
if os.getenv("PROD_STAGE") == "TRUE": | ||
print("In production environment, config not overridden, using default production debug.conf") | ||
config_file = open('conf/analysis/debug.conf.prod.json') | ||
else: | ||
print("analysis.debug.conf.json not configured, falling back to sample, default configuration") | ||
config_file = open('conf/analysis/debug.conf.dev.json') | ||
ret_val = json.load(config_file) | ||
config_file.close() | ||
return ret_val | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
import json | ||
import logging | ||
import os | ||
import numpy as np | ||
import pandas as pd | ||
|
||
# if there is a config file and the environment variable is set, we need to | ||
# decide which one wins. I would argue for the environment variable, to allow | ||
# for a migration to the new model and for us to remove the obsolete code. | ||
# Although arguably, the converse will also work, since we can set the | ||
# variable while the file is present, and then remove the file in a second | ||
# round of changes. Let's keep the order unchanged for now for simplicity, and | ||
# modify as needed later. | ||
|
||
def get_config(config_file_name, var_path_mapping): | ||
# Since a `config_data` field would be at the module level, and we want | ||
# the module to be reusable, we are not going to cache the result. It is | ||
# not clear that we need to cache the result anyway, given that we | ||
# typically initialize the config variables at the beginning of the | ||
# modules in which they are used. If we feel like this is an issue, we can | ||
# switch to creating a class instead. | ||
ret_val = {} | ||
try: | ||
config_file = open(config_file_name) | ||
# we only have a single entry in the config json, not an array | ||
# and there is no way for json_normalize to return a series | ||
# so we will just take the first row of the dataframe | ||
loaded_val = pd.json_normalize(json.load(config_file)).iloc[0] | ||
for var, path in var_path_mapping.items(): | ||
ret_val[var] = loaded_val[path] | ||
# Ensure that the returned values are regular ints | ||
# https://github.com/e-mission/e-mission-server/pull/961#issuecomment-2282206511 | ||
if type(ret_val[var]) is np.int64: | ||
ret_val[var] = int(ret_val[var]) | ||
config_file.close() | ||
except Exception as e: | ||
if isinstance(e, KeyError) or isinstance(e, json.decoder.JSONDecodeError): | ||
logging.exception(e) | ||
print("Config file not found, returning a copy of the environment variables instead...") | ||
# https://github.com/e-mission/e-mission-server/pull/961#issuecomment-2282209006 | ||
ret_val = dict(os.environ) | ||
return ret_val |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need the
tag_file.txt
to be uploaded here given that we are passing it directly to the workflows on line 77?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See Mukul's comment in the issue:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the issue here. You have to deal with dispatch and push separately anyway - one of them will have the image tag and the other will not. And given that we have the
.env
file checked in now, the push can just use that directly. I don't see why we need Yet Another file being uploaded from the workflow. Having said that, I am not going to hold up this PR for this, but it needs to be addressed as a polishing change.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do see now why we do not need the artifact for the push event. Detailed comments made here in public-dash Redesign PR. The comments talk about the corresponding artifact download in the dashboard repos and the explanation is applicable to the upload artifact on the server side as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MukuFlash03 I utilize the artifacts to get the tags to the internal repo, so please don't remove them yet!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nataliejschultz For now, I've tested removing only the
Download artifacts
step in the public-dashboard workflow.I don't think it is related to the
Upload artifacts
step that you added for internal repo.Neither am I making changes to the server workflow.
Can you please confirm if in this case it is alright to remove the artifact dealing with Push event from the dashboard workflows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MukuFlash03 sorry, I misunderstood what you were changing. It is okay with me to remove the
download artifacts
step!